diff mbox

[v2,1/2] fs: Improve and simplify copy_mount_options

Message ID 20160616054525.GA6803@1wt.eu (mailing list archive)
State New, archived
Headers show

Commit Message

Willy Tarreau June 16, 2016, 5:45 a.m. UTC 
On Wed, Jun 15, 2016 at 02:42:33PM -1000, Linus Torvalds wrote:
> On Wed, Jun 15, 2016 at 2:01 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > devtmpfsd does:
> >
> >         *err = sys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, options);
> >
> > where options points to the kernel stack.  This is bad.  do_mount_root
> > is similarly broken.
> >
> > Is there any reason that these things use sys_mount instead of do_mount?
> 
> Not that I can see. But maybe copy_mount_options could also check for
> KERNEL_DS, and use a strncpy instead of a copy_from_user() for that
> case?

Well, strncpy() would make the function behave differently depending on
the FS being used if called from the kernel for the reason Al mentionned.
OK devtmpfsd() passes a string, but if it's the FS itself which decides
to stop on a zero when parsing mount options, we'd probably rather use
memcpy() instead to ensure a consistent behaviour, like this maybe ?

Willy


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Torvalds June 16, 2016, 5:59 a.m. UTC | #1 
On Wed, Jun 15, 2016 at 7:45 PM, Willy Tarreau <w@1wt.eu> wrote:
>
> Well, strncpy() would make the function behave differently depending on
> the FS being used if called from the kernel for the reason Al mentionned.
> OK devtmpfsd() passes a string, but if it's the FS itself which decides
> to stop on a zero when parsing mount options, we'd probably rather use
> memcpy() instead to ensure a consistent behaviour, like this maybe ?

.. but that is exactly what Andy considers to be a problem: now it
copies random kernel memory that is possibly security-critical.

The kernel users that use this just pass in a string - it doesn't
matter what the filesystem thinks it is getting, the uses were all
kernel strings,, so the "copy_mount_options": should copy that string
(and zero-fill the page that the filesystem may think it is getting).

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691..058b856 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2622,6 +2622,12 @@  void *copy_mount_options(const void __user * data)
 	if (!copy)
 		return ERR_PTR(-ENOMEM);
 
+	/* do_mount() may be called from the kernel */
+	if (segment_eq(get_fs(), KERNEL_DS)) {
+		memcpy(copy, data, PAGE_SIZE);
+		return copy;
+	}
+
 	/* We only care that *some* data at the address the user
 	 * gave us is valid.  Just in case, we'll zero
 	 * the remainder of the page.