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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171007020217.GN21978@ZenIV.linux.org.uk>
Date:   Sat, 7 Oct 2017 03:02:17 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Jia-Ju Bai <baijiaju1990@....com>, Jan Kara <jack@...e.com>,
        Sagi Grimberg <sagi@...mberg.me>, james.smart@...adcom.com,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in
 parse_options

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@....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...