[PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options

[PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options

[Jia-Ju Bai]

The kernel may sleep under a spinlock, and the function call path is:
ext2_remount
  parse_options
    match_int
      match_number (lib/parser.c)
        kmalloc(GFP_KERNEL) --> may sleep

To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
This bug is found by my static analysis tool and my code review.

Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
 lib/parser.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/parser.c b/lib/parser.c
index 3278958..bc6e2ce 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -133,7 +133,7 @@ static int match_number(substring_t *s, int *result, int base)
 	long val;
 	size_t len = s->to - s->from;
 
-	buf = kmalloc(len + 1, GFP_KERNEL);
+	buf = kmalloc(len + 1, GFP_ATOMIC);
 	if (!buf)
 		return -ENOMEM;
 	memcpy(buf, s->from, len);
-- 
1.7.9.5

Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options

[Linus Torvalds]

On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
>
> To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
> This bug is found by my static analysis tool and my code review.

I'm not saying your patch is wrong, but it's a shame that we do that
extra allocation in match_number() and match_u64int(), and that we
don't have anything that is just size-limited.

And there really isn't anything saying that we shouldn't do the same
silly thing to match_u64int(). Maybe we don't have any actual users
that need it for now, but still..

Oh well.

I do wonder if we shouldn't just use something like

 "skip leading zeroes, copy to size-limited stack location instead"

because the input length really *is* limited once you skip leading
zeroes (and whatever base marker we have). We might have at most a
64-bit value in octal, so 22 bytes max.

But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.

               Linus

Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options

[Jia-Ju Bai]

Thanks for your reply.
I agree that extra allocation in match_number() and match_u64int() may 
be unnecessary.

Thanks,
Jia-Ju Bai


On 2017/10/7 9:37, Linus Torvalds wrote:
> On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
>> To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
>> This bug is found by my static analysis tool and my code review.
> I'm not saying your patch is wrong, but it's a shame that we do that
> extra allocation in match_number() and match_u64int(), and that we
> don't have anything that is just size-limited.
>
> And there really isn't anything saying that we shouldn't do the same
> silly thing to match_u64int(). Maybe we don't have any actual users
> that need it for now, but still..
>
> Oh well.
>
> I do wonder if we shouldn't just use something like
>
>   "skip leading zeroes, copy to size-limited stack location instead"
>
> because the input length really *is* limited once you skip leading
> zeroes (and whatever base marker we have). We might have at most a
> 64-bit value in octal, so 22 bytes max.
>
> But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.
>
>                 Linus

Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options

[Al Viro]

On Fri, Oct 06, 2017 at 06:37:11PM -0700, Linus Torvalds wrote:
> On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
> >
> > To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
> > This bug is found by my static analysis tool and my code review.
> 
> I'm not saying your patch is wrong, but it's a shame that we do that
> extra allocation in match_number() and match_u64int(), and that we
> don't have anything that is just size-limited.
> 
> And there really isn't anything saying that we shouldn't do the same
> silly thing to match_u64int(). Maybe we don't have any actual users
> that need it for now, but still..
> 
> Oh well.
> 
> I do wonder if we shouldn't just use something like
> 
>  "skip leading zeroes, copy to size-limited stack location instead"
> 
> because the input length really *is* limited once you skip leading
> zeroes (and whatever base marker we have). We might have at most a
> 64-bit value in octal, so 22 bytes max.
> 
> But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.

	There's match_strdup() as well...

	FWIW, ext2 side also looks fishy; it might be cleaner if we
collected new state into some object and applied it only after the last
possible failure exit.  The entire "restore the original state" logics
would go away...

Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options

[Al Viro]

On Sat, Oct 07, 2017 at 03:02:17AM +0100, Al Viro wrote:
> > I do wonder if we shouldn't just use something like
> > 
> >  "skip leading zeroes, copy to size-limited stack location instead"
> > 
> > because the input length really *is* limited once you skip leading
> > zeroes (and whatever base marker we have). We might have at most a
> > 64-bit value in octal, so 22 bytes max.
> > 
> > But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.
> 
> 	There's match_strdup() as well...
> 
> 	FWIW, ext2 side also looks fishy; it might be cleaner if we
> collected new state into some object and applied it only after the last
> possible failure exit.  The entire "restore the original state" logics
> would go away...

	I'm not saying that the bug had been introduced by conversion to
spinlock, BTW - it was racy back when ext2_remount() relied upon BKL.
I hadn't considered the atomicity issues back then - mea culpa...

Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options

[Jan Kara]

On Sat 07-10-17 03:02:17, Al Viro wrote:
> On Fri, Oct 06, 2017 at 06:37:11PM -0700, Linus Torvalds wrote:
> > On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
> > >
> > > To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
> > > This bug is found by my static analysis tool and my code review.
> > 
> > I'm not saying your patch is wrong, but it's a shame that we do that
> > extra allocation in match_number() and match_u64int(), and that we
> > don't have anything that is just size-limited.
> > 
> > And there really isn't anything saying that we shouldn't do the same
> > silly thing to match_u64int(). Maybe we don't have any actual users
> > that need it for now, but still..
> > 
> > Oh well.
> > 
> > I do wonder if we shouldn't just use something like
> > 
> >  "skip leading zeroes, copy to size-limited stack location instead"
> > 
> > because the input length really *is* limited once you skip leading
> > zeroes (and whatever base marker we have). We might have at most a
> > 64-bit value in octal, so 22 bytes max.
> > 
> > But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.
> 
> 	There's match_strdup() as well...
> 
> 	FWIW, ext2 side also looks fishy; it might be cleaner if we
> collected new state into some object and applied it only after the last
> possible failure exit.  The entire "restore the original state" logics
> would go away...

Well, it's not like the restore logic would be that difficult for ext2. But
I agree that running the whole parsing logic under a spinlock is
unnecessary and accumulating all the changes in one structure and then
applying them looks like a cleaner way to go. I'll look into that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options

[Dave Chinner]

On Sat, Oct 07, 2017 at 09:20:46AM +0800, Jia-Ju Bai wrote:
> The kernel may sleep under a spinlock, and the function call path is:
> ext2_remount
>   parse_options
>     match_int
>       match_number (lib/parser.c)
>         kmalloc(GFP_KERNEL) --> may sleep
> 
> To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
> This bug is found by my static analysis tool and my code review.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> ---
>  lib/parser.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/parser.c b/lib/parser.c
> index 3278958..bc6e2ce 100644
> --- a/lib/parser.c
> +++ b/lib/parser.c
> @@ -133,7 +133,7 @@ static int match_number(substring_t *s, int *result, int base)
>  	long val;
>  	size_t len = s->to - s->from;
>  
> -	buf = kmalloc(len + 1, GFP_KERNEL);
> +	buf = kmalloc(len + 1, GFP_ATOMIC);

That seems like the wrong thing to do.

The problem is that ext2_remount is running it's internal
parse_options() under a spinlock, rather than doing the parsing with
no locks held and then only taking the locks when it needs to change
the superblock state.

At a quick glance, I don't see any other filesystem with the same
problem....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com