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: [email protected]
Tested-by: [email protected]
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Hawkins Jiawei <[email protected]>
---
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;
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
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: [email protected]
> Tested-by: [email protected]
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Hawkins Jiawei <[email protected]>
Thank you!
This was on my list of things to look at.
Do you know if the issue existed before commit ac369cdd9448? Just
wondering if we need a Fixes tag and stable backports.
I'll take a closer look at the patch a bit later, unless someone beats
me to it.
--
Mike Kravetz
> ---
> 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;
> 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
>
Hi Mike,
On Fri, 21 Oct 2022 at 07:38, Mike Kravetz <[email protected]> wrote:
>
> 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: [email protected]
> > Tested-by: [email protected]
> > Link: https://lore.kernel.org/all/[email protected]/
> > Signed-off-by: Hawkins Jiawei <[email protected]>
>
> Thank you!
>
> This was on my list of things to look at.
>
> Do you know if the issue existed before commit ac369cdd9448? Just
> wondering if we need a Fixes tag and stable backports.
This issue did not exist before commit ac369cdd9448, because kernel will
sets param->string a string(although a zero length string) by kmemdup_nul()
when fs string has zero length in vfs_parse_fs_string(). So it will ensures
that param->string is not a null pointer in hugetlbfs_parse_param().
I also revert commit ac369cdd9448 or reset to ac369cdd9448~, and test
the syzkaller reproducer. The reproducer does not trigger any issue
in both situations.
>
> I'll take a closer look at the patch a bit later, unless someone beats
> me to it.
> --
> Mike Kravetz
>
> > ---
> > 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;
> > 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
> >
On Thu, 20 Oct 2022 16:38:18 -0700 Mike Kravetz <[email protected]> wrote:
> 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: [email protected]
> > Tested-by: [email protected]
> > Link: https://lore.kernel.org/all/[email protected]/
> > Signed-off-by: Hawkins Jiawei <[email protected]>
>
> Thank you!
>
> This was on my list of things to look at.
>
> Do you know if the issue existed before commit ac369cdd9448? Just
> wondering if we need a Fixes tag and stable backports.
Ian Kent's "vfs: parse: deal with zero length string value" exists only
in mm-nonmm-unstable. I'm hoping that it gets some attention from Al
Viro.
I'll merge this patch so we're ready for Ian's patch. I normally merge
mm-stable ahead of mm-nonmm-stable so there shouldn't be a bisection
window.
Ian, could you please go through the filesystems and see if there are
other instances of this getting ready to bite us?
> I'll take a closer look at the patch a bit later, unless someone beats
> me to it.
> --
> Mike Kravetz
>
> > ---
> > 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;
> > 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
> >
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: [email protected]
> Tested-by: [email protected]
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Hawkins Jiawei <[email protected]>
> ---
> 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 <[email protected]>
--
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
>