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@xxxxxxxxxx>
> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
>
> 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
