Re: [PATCH -next] hugetlbfs: fix null-ptr-deref in hugetlbfs_parse_param()

[Mike Kravetz]

On 10/21/22 07:16, Hawkins Jiawei wrote:
> Syzkaller reports a null-ptr-deref bug as follows:
> ======================================================
> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> RIP: 0010:hugetlbfs_parse_param+0x1dd/0x8e0 fs/hugetlbfs/inode.c:1380
> [...]
> Call Trace:
>  <TASK>
>  vfs_parse_fs_param fs/fs_context.c:148 [inline]
>  vfs_parse_fs_param+0x1f9/0x3c0 fs/fs_context.c:129
>  vfs_parse_fs_string+0xdb/0x170 fs/fs_context.c:191
>  generic_parse_monolithic+0x16f/0x1f0 fs/fs_context.c:231
>  do_new_mount fs/namespace.c:3036 [inline]
>  path_mount+0x12de/0x1e20 fs/namespace.c:3370
>  do_mount fs/namespace.c:3383 [inline]
>  __do_sys_mount fs/namespace.c:3591 [inline]
>  __se_sys_mount fs/namespace.c:3568 [inline]
>  __x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>  [...]
>  </TASK>
> ======================================================
> 
> According to commit
> ac369cdd9448("vfs: parse: deal with zero length string value"),
> kernel will sets the param->string to null pointer in
> vfs_parse_fs_string() if fs string has zero length.
> 
> Yet the problem is that, hugetlbfs_parse_param() will
> dereference the param->string, without checking whether it
> is a null pointer. To be more specific, if hugetlbfs_parse_param()
> parses an illegal mount parameter, such as "size=,", kernel will
> constructs struct fs_parameter with null pointer in
> vfs_parse_fs_string(), then passes this struct fs_parameter to
> hugetlbfs_parse_param(), which triggers the above
> null-ptr-deref bug.
> 
> This patch solves it by adding sanity check on param->string
> in hugetlbfs_parse_param().
> 
> Reported-by: syzbot+a3e6acd85ded5c16a709@xxxxxxxxxxxxxxxxxxxxxxxxx
> Tested-by: syzbot+a3e6acd85ded5c16a709@xxxxxxxxxxxxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/all/0000000000005ad00405eb7148c6@xxxxxxxxxx/
> Signed-off-by: Hawkins Jiawei <yin31149@xxxxxxxxx>
> ---
>  fs/hugetlbfs/inode.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 7f836f8f9db1..3ee84604e36d 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -1377,7 +1377,7 @@ static int hugetlbfs_parse_param(struct fs_context *fc, struct fs_parameter *par
>  
>  	case Opt_size:
>  		/* memparse() will accept a K/M/G without a digit */
> -		if (!isdigit(param->string[0]))
> +		if (!param->string || !isdigit(param->string[0]))
>  			goto bad_val;

At the bad_val label we have,

bad_val:
        return invalfc(fc, "Bad value '%s' for mount option '%s'\n",
                      param->string, param->key);

param->string still is NULL.  But, I assume the logging can handle this.
In the case of printk, a NULL pointer comes out as '(null)'.

Thanks again,

Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
-- 
Mike Kravetz


>  		ctx->max_size_opt = memparse(param->string, &rest);
>  		ctx->max_val_type = SIZE_STD;
> @@ -1387,7 +1387,7 @@ static int hugetlbfs_parse_param(struct fs_context *fc, struct fs_parameter *par
>  
>  	case Opt_nr_inodes:
>  		/* memparse() will accept a K/M/G without a digit */
> -		if (!isdigit(param->string[0]))
> +		if (!param->string || !isdigit(param->string[0]))
>  			goto bad_val;
>  		ctx->nr_inodes = memparse(param->string, &rest);
>  		return 0;
> @@ -1403,7 +1403,7 @@ static int hugetlbfs_parse_param(struct fs_context *fc, struct fs_parameter *par
>  
>  	case Opt_min_size:
>  		/* memparse() will accept a K/M/G without a digit */
> -		if (!isdigit(param->string[0]))
> +		if (!param->string || !isdigit(param->string[0]))
>  			goto bad_val;
>  		ctx->min_size_opt = memparse(param->string, &rest);
>  		ctx->min_val_type = SIZE_STD;
> -- 
> 2.25.1
>