From: Vivek Goyal <vgoyal@redhat.com>
To: Christoph Hellwig <hch@infradead.org>, Jan Kara <jack@suse.cz>,
viro@zeniv.linux.org.uk
Cc: Linux fsdevel mailing list <linux-fsdevel@vger.kernel.org>,
linux kernel mailing list <linux-kernel@vger.kernel.org>,
xu.xin16@zte.com.cn, christian.brauner@ubuntu.com
Subject: [PATCH v3] init/do_mounts.c: Harden split_fs_names() against buffer overflow
Date: Fri, 17 Sep 2021 09:13:23 -0400 [thread overview]
Message-ID: <YUSUc7AyCq/P3SLR@redhat.com> (raw)
split_fs_names() currently takes comma separate list of filesystems
and converts it into individual filesystem strings. Pleaces these
strings in the input buffer passed by caller and returns number of
strings.
If caller manages to pass input string bigger than buffer, then we
can write beyond the buffer. Or if string just fits buffer, we will
still write beyond the buffer as we append a '\0' byte at the end.
Pass size of input buffer to split_fs_names() and put enough checks
in place so such buffer overrun possibilities do not occur.
This patch does few things.
- Add a parameter "size" to split_fs_names(). This specifies size
of input buffer.
- Use strlcpy() (instead of strcpy()) so that we can't go beyond
buffer size. If input string "names" is larger than passed in
buffer, input string will be truncated to fit in buffer.
- Stop appending extra '\0' character at the end and avoid one
possibility of going beyond the input buffer size.
- Do not use extra loop to count number of strings.
- Previously if one passed "rootfstype=foo,,bar", split_fs_names()
will return only 1 string "foo" (and "bar" will be truncated
due to extra ,). After this patch, now split_fs_names() will
return 3 strings ("foo", zero-sized-string, and "bar").
Callers of split_fs_names() have been modified to check for
zero sized string and skip to next one.
Reported-by: xu xin <xu.xin16@zte.com.cn>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
Changes from v2:
- Got rid of str_start variable. (Jan Kara)
---
init/do_mounts.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
Index: redhat-linux/init/do_mounts.c
===================================================================
--- redhat-linux.orig/init/do_mounts.c 2021-09-15 08:46:33.801689806 -0400
+++ redhat-linux/init/do_mounts.c 2021-09-17 08:44:40.781430167 -0400
@@ -338,20 +338,19 @@ __setup("rootflags=", root_data_setup);
__setup("rootfstype=", fs_names_setup);
__setup("rootdelay=", root_delay_setup);
-static int __init split_fs_names(char *page, char *names)
+/* This can return zero length strings. Caller should check */
+static int __init split_fs_names(char *page, size_t size, char *names)
{
- int count = 0;
+ int count = 1;
char *p = page;
- strcpy(p, root_fs_names);
+ strlcpy(p, root_fs_names, size);
while (*p++) {
- if (p[-1] == ',')
+ if (p[-1] == ',') {
p[-1] = '\0';
+ count++;
+ }
}
- *p = '\0';
-
- for (p = page; *p; p += strlen(p)+1)
- count++;
return count;
}
@@ -404,12 +403,16 @@ void __init mount_block_root(char *name,
scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
if (root_fs_names)
- num_fs = split_fs_names(fs_names, root_fs_names);
+ num_fs = split_fs_names(fs_names, PAGE_SIZE, root_fs_names);
else
num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
retry:
for (i = 0, p = fs_names; i < num_fs; i++, p += strlen(p)+1) {
- int err = do_mount_root(name, p, flags, root_mount_data);
+ int err;
+
+ if (!*p)
+ continue;
+ err = do_mount_root(name, p, flags, root_mount_data);
switch (err) {
case 0:
goto out;
@@ -543,10 +546,12 @@ static int __init mount_nodev_root(void)
fs_names = (void *)__get_free_page(GFP_KERNEL);
if (!fs_names)
return -EINVAL;
- num_fs = split_fs_names(fs_names, root_fs_names);
+ num_fs = split_fs_names(fs_names, PAGE_SIZE, root_fs_names);
for (i = 0, fstype = fs_names; i < num_fs;
i++, fstype += strlen(fstype) + 1) {
+ if (!*fstype)
+ continue;
if (!fs_is_nodev(fstype))
continue;
err = do_mount_root(root_device_name, fstype, root_mountflags,
next reply other threads:[~2021-09-17 13:13 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-17 13:13 Vivek Goyal [this message]
2021-09-20 2:23 ` [PATCH v3] init/do_mounts.c: Harden split_fs_names() against buffer overflow Al Viro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YUSUc7AyCq/P3SLR@redhat.com \
--to=vgoyal@redhat.com \
--cc=christian.brauner@ubuntu.com \
--cc=hch@infradead.org \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=xu.xin16@zte.com.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.