On Fri, Sep 17, 2021 at 10:07:30AM +0200, Jan Kara wrote:
> On Thu 16-09-21 11:50:58, Vivek Goyal wrote:
> > 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@xxxxxxxxxx>
> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > ---
> > init/do_mounts.c | 28 ++++++++++++++++++++--------
> > 1 file changed, 20 insertions(+), 8 deletions(-)
>
> Just one nit 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-16 11:28:36.753625037 -0400
> > @@ -338,19 +338,25 @@ __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;
> > + bool str_start = false;
> >
> > - strcpy(p, root_fs_names);
> > + strlcpy(p, root_fs_names, size);
> > while (*p++) {
> > - if (p[-1] == ',')
> > + if (p[-1] == ',') {
> > p[-1] = '\0';
> > + count++;
> > + str_start = false;
> > + } else {
> > + str_start = true;
> > + }
> > }
> > - *p = '\0';
> >
> > - for (p = page; *p; p += strlen(p)+1)
> > + /* Last string which might not be comma terminated */
> > + if (str_start)
> > count++;
>
> You could avoid the whole str_start logic if you just initialize 'count' to
> 1 - in the worst case you'll have 0-length string at the end (for case like
> xfs,) but you deal with 0-length strings in the callers anyway. Otherwise
> the patch looks good so feel free to add:
Hi Jan,
This sounds good. I will get rid of str_start. V3 of the patch is on
the way.
Vivek
>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
>
