| Message ID | YUIPnPV2ttOHNIcX@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | init/do_mounts.c: Harden split_fs_names() against buffer overflow | expand |
On Wed, Sep 15, 2021 at 11:22:04AM -0400, Vivek Goyal wrote: > Will be nice to pass size of input buffer to split_fs_names() and > put enough checks in place so such buffer overrun possibilities > do not occur. Will be nice sounds weird. > > Hence this patch adds "size" parameter to split_fs_names() and makes > sure we do not access memory beyond size. If input string "names" > is larger than passed in buffer, input string will be truncated to > fit in buffer. There's really two aspects here: checking for a max size and explicitly passing one. I'm fine with passing the argument even if it always is PAGE_SIZE, but this should probably be documented a little better. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Sep 15, 2021 at 11:22:04AM -0400, Vivek Goyal wrote: > split_fs_names() currently takes comma separated 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. > > Will be nice to pass size of input buffer to split_fs_names() and > put enough checks in place so such buffer overrun possibilities > do not occur. > > Hence this patch adds "size" parameter to split_fs_names() and makes > sure we do not access memory beyond size. If input string "names" > is larger than passed in buffer, input string will be truncated to > fit in buffer. > > Reported-by: xu xin <xu.xin16@zte.com.cn> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- Strange but probably reasonable, Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
On Wed 15-09-21 11:22:04, Vivek Goyal wrote: > split_fs_names() currently takes comma separated 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. > > Will be nice to pass size of input buffer to split_fs_names() and > put enough checks in place so such buffer overrun possibilities > do not occur. > > Hence this patch adds "size" parameter to split_fs_names() and makes > sure we do not access memory beyond size. If input string "names" > is larger than passed in buffer, input string will be truncated to > fit in buffer. > > Reported-by: xu xin <xu.xin16@zte.com.cn> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> The patch looks correct but IMO is more complicated than it needs to be... See below. > 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-15 09:52:09.884449718 -0400 > @@ -338,19 +338,20 @@ __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) > +static int __init split_fs_names(char *page, size_t size, char *names) > { > int count = 0; > - char *p = page; > + char *p = page, *end = page + size - 1; > + > + strncpy(p, root_fs_names, size); Why not strlcpy()? That way you don't have to explicitely terminate the string... > + *end = '\0'; > > - strcpy(p, root_fs_names); > while (*p++) { > if (p[-1] == ',') > p[-1] = '\0'; > } > - *p = '\0'; > > - for (p = page; *p; p += strlen(p)+1) > + for (p = page; p < end && *p; p += strlen(p)+1) > count++; And I kind of fail to see why you have a separate loop for counting number of elements when you could count them directly when changing ',' to '\0'. There's this small subtlety that e.g. string 'foo,,bar' will report to have only 1 element with the above code while direct computation would return 3 but that's hardly problem IMHO. Honza
On Thu, Sep 16, 2021 at 01:00:16PM +0200, Jan Kara wrote: > On Wed 15-09-21 11:22:04, Vivek Goyal wrote: > > split_fs_names() currently takes comma separated 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. > > > > Will be nice to pass size of input buffer to split_fs_names() and > > put enough checks in place so such buffer overrun possibilities > > do not occur. > > > > Hence this patch adds "size" parameter to split_fs_names() and makes > > sure we do not access memory beyond size. If input string "names" > > is larger than passed in buffer, input string will be truncated to > > fit in buffer. > > > > Reported-by: xu xin <xu.xin16@zte.com.cn> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > The patch looks correct but IMO is more complicated than it needs to be... > See below. > > > 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-15 09:52:09.884449718 -0400 > > @@ -338,19 +338,20 @@ __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) > > +static int __init split_fs_names(char *page, size_t size, char *names) > > { > > int count = 0; > > - char *p = page; > > + char *p = page, *end = page + size - 1; > > + > > + strncpy(p, root_fs_names, size); > > Why not strlcpy()? That way you don't have to explicitely terminate the > string... Sure, will use strlcpy(). > > > + *end = '\0'; > > > > - strcpy(p, root_fs_names); > > while (*p++) { > > if (p[-1] == ',') > > p[-1] = '\0'; > > } > > - *p = '\0'; > > > > - for (p = page; *p; p += strlen(p)+1) > > + for (p = page; p < end && *p; p += strlen(p)+1) > > count++; > > And I kind of fail to see why you have a separate loop for counting number > of elements when you could count them directly when changing ',' to '\0'. > There's this small subtlety that e.g. string 'foo,,bar' will report to have > only 1 element with the above code while direct computation would return 3 > but that's hardly problem IMHO. Ok, will make this change. One side affect of this change will be that now split_fs_names() can return zero sized strings and caller will have to check for those and skip to next string. Vivek
On Thu 16-09-21 11:41:53, Vivek Goyal wrote: > On Thu, Sep 16, 2021 at 01:00:16PM +0200, Jan Kara wrote: > > On Wed 15-09-21 11:22:04, Vivek Goyal wrote: > > > split_fs_names() currently takes comma separated 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. > > > > > > Will be nice to pass size of input buffer to split_fs_names() and > > > put enough checks in place so such buffer overrun possibilities > > > do not occur. > > > > > > Hence this patch adds "size" parameter to split_fs_names() and makes > > > sure we do not access memory beyond size. If input string "names" > > > is larger than passed in buffer, input string will be truncated to > > > fit in buffer. > > > > > > Reported-by: xu xin <xu.xin16@zte.com.cn> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > > The patch looks correct but IMO is more complicated than it needs to be... > > See below. > > > > > 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-15 09:52:09.884449718 -0400 > > > @@ -338,19 +338,20 @@ __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) > > > +static int __init split_fs_names(char *page, size_t size, char *names) > > > { > > > int count = 0; > > > - char *p = page; > > > + char *p = page, *end = page + size - 1; > > > + > > > + strncpy(p, root_fs_names, size); > > > > Why not strlcpy()? That way you don't have to explicitely terminate the > > string... > > Sure, will use strlcpy(). > > > > > > + *end = '\0'; > > > > > > - strcpy(p, root_fs_names); > > > while (*p++) { > > > if (p[-1] == ',') > > > p[-1] = '\0'; > > > } > > > - *p = '\0'; > > > > > > - for (p = page; *p; p += strlen(p)+1) > > > + for (p = page; p < end && *p; p += strlen(p)+1) > > > count++; > > > > And I kind of fail to see why you have a separate loop for counting number > > of elements when you could count them directly when changing ',' to '\0'. > > There's this small subtlety that e.g. string 'foo,,bar' will report to have > > only 1 element with the above code while direct computation would return 3 > > but that's hardly problem IMHO. > > Ok, will make this change. One side affect of this change will be that now > split_fs_names() can return zero sized strings and caller will have > to check for those and skip to next string. Or we can just abort the loop early and don't bother with converting further ',' if 0-length strings are indeed any problem. Honza
On Thu, Sep 16, 2021 at 06:54:46PM +0200, Jan Kara wrote: > On Thu 16-09-21 11:41:53, Vivek Goyal wrote: > > On Thu, Sep 16, 2021 at 01:00:16PM +0200, Jan Kara wrote: > > > On Wed 15-09-21 11:22:04, Vivek Goyal wrote: > > > > split_fs_names() currently takes comma separated 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. > > > > > > > > Will be nice to pass size of input buffer to split_fs_names() and > > > > put enough checks in place so such buffer overrun possibilities > > > > do not occur. > > > > > > > > Hence this patch adds "size" parameter to split_fs_names() and makes > > > > sure we do not access memory beyond size. If input string "names" > > > > is larger than passed in buffer, input string will be truncated to > > > > fit in buffer. > > > > > > > > Reported-by: xu xin <xu.xin16@zte.com.cn> > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > > > > The patch looks correct but IMO is more complicated than it needs to be... > > > See below. > > > > > > > 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-15 09:52:09.884449718 -0400 > > > > @@ -338,19 +338,20 @@ __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) > > > > +static int __init split_fs_names(char *page, size_t size, char *names) > > > > { > > > > int count = 0; > > > > - char *p = page; > > > > + char *p = page, *end = page + size - 1; > > > > + > > > > + strncpy(p, root_fs_names, size); > > > > > > Why not strlcpy()? That way you don't have to explicitely terminate the > > > string... > > > > Sure, will use strlcpy(). > > > > > > > > > + *end = '\0'; > > > > > > > > - strcpy(p, root_fs_names); > > > > while (*p++) { > > > > if (p[-1] == ',') > > > > p[-1] = '\0'; > > > > } > > > > - *p = '\0'; > > > > > > > > - for (p = page; *p; p += strlen(p)+1) > > > > + for (p = page; p < end && *p; p += strlen(p)+1) > > > > count++; > > > > > > And I kind of fail to see why you have a separate loop for counting number > > > of elements when you could count them directly when changing ',' to '\0'. > > > There's this small subtlety that e.g. string 'foo,,bar' will report to have > > > only 1 element with the above code while direct computation would return 3 > > > but that's hardly problem IMHO. > > > > Ok, will make this change. One side affect of this change will be that now > > split_fs_names() can return zero sized strings and caller will have > > to check for those and skip to next string. > > Or we can just abort the loop early and don't bother with converting > further ',' if 0-length strings are indeed any problem. There are only two callers of split_fs_names(). So changing them for zero sized strings was trivial (patch v2). So I peronally don't mind supporting "rootfstype=xfs,,ext4" if there is an accidental extra ',' in there. Vivek
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-15 09:52:09.884449718 -0400 @@ -338,19 +338,20 @@ __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) +static int __init split_fs_names(char *page, size_t size, char *names) { int count = 0; - char *p = page; + char *p = page, *end = page + size - 1; + + strncpy(p, root_fs_names, size); + *end = '\0'; - strcpy(p, root_fs_names); while (*p++) { if (p[-1] == ',') p[-1] = '\0'; } - *p = '\0'; - for (p = page; *p; p += strlen(p)+1) + for (p = page; p < end && *p; p += strlen(p)+1) count++; return count; @@ -404,7 +405,7 @@ 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: @@ -543,7 +544,7 @@ 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) {
split_fs_names() currently takes comma separated 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. Will be nice to pass size of input buffer to split_fs_names() and put enough checks in place so such buffer overrun possibilities do not occur. Hence this patch adds "size" parameter to split_fs_names() and makes sure we do not access memory beyond size. If input string "names" is larger than passed in buffer, input string will be truncated to fit in buffer. Reported-by: xu xin <xu.xin16@zte.com.cn> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- init/do_mounts.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)