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:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
