Re: [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing param

[Ian Kent]

On 31/10/22 19:28, Tetsuo Handa wrote:
> On 2022/10/24 12:34, Ian Kent wrote:
> > On 24/10/22 08:42, Hawkins Jiawei wrote:
> > > On Mon, 24 Oct 2022 at 00:48, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> > > > On Mon, Oct 24, 2022 at 12:39:41AM +0800, Hawkins Jiawei wrote:
> > > > > According to commit "vfs: parse: deal with zero length string value",
> > > > > kernel will set the param->string to null pointer in vfs_parse_fs_string()
> > > > > if fs string has zero length.
> > > > >
> > > > > Yet the problem is that, when fs parses its mount parameters, it will
> > > > > dereferences the param->string, without checking whether it is a
> > > > > null pointer, which may trigger a null-ptr-deref bug.
> > > > >
> > > > > So this patchset reviews all functions for fs to parse parameters,
> > > > > by using `git grep -n "\.parse_param" fs/*`, and adds sanity check
> > > > > on param->string if its function will dereference param->string
> > > > > without check.
> > > > How about reverting the commit in question instead?  Or dropping it
> > > > from patch series, depending upon the way akpm handles the pile
> > > > these days...
> > > I think both are OK.
> > >
> > > On one hand, commit "vfs: parse: deal with zero length string value"
> > > seems just want to make output more informattive, which probably is not
> > > the one which must be applied immediately to fix the
> > > panic.
> > >
> > > On the other hand, commit "vfs: parse: deal with zero length string value"
> > > affects so many file systems, so there are probably some deeper
> > > null-ptr-deref bugs I ignore, which may take time to review.
> > Yeah, it would be good to make the file system handling consistent
> > but I think there's been a bit too much breakage and it appears not
> > everyone thinks the approach is the right way to do it.
> >
> > I'm thinking of abandoning this and restricting it to the "source"
> > parameter only to solve the user space mount table parser problem but
> > still doing it in the mount context code to keep it general (at least
> > for this case).
> No progress on this problem, and syzbot is reporting one after the other...
>
> I think that reverting is the better choice.

Yes, I agree/


Ian