Re: [PATCH 1/1] Revert "iomap: fall back to buffered writes for invalidation failures"

[Lee Jones]

On Wed, 09 Feb 2022, Darrick J. Wong wrote:

> On Wed, Feb 09, 2022 at 08:52:43AM +0000, Lee Jones wrote:
> > This reverts commit 60263d5889e6dc5987dc51b801be4955ff2e4aa7.
> > 
> > Reverting since this commit opens a potential avenue for abuse.
> 
> What kind of abuse?  Did you conclude there's an avenue solely because
> some combination of userspace rigging produced a BUG warning?  Or is
> this a real problem that someone found?

Genuine question: Is the ability for userspace to crash the kernel
not enough to cause concern?  I would have thought that we'd want to
prevent this.

If by 'real problem' you mean; privilege escalation, memory corruption
or data leakage, then no, I haven't found any evidence of that.
However, that's not to say these aren't possible as a result of this
issue, just that I do not have the skills or knowledge to be able to
turn this into a demonstrable attack vector.

However, if you say there is no issue, I'm happy to take your word.

> > The C-reproducer and more information can be found at the link below.
> 
> No.  Post the information and your analysis here.  I'm not going to dig
> into some Google site to find out what happened, and I'm not going to
> assume that future developers will be able to access that URL to learn
> why this patch was created.

The link provided doesn't contain any further analysis.  Only the
reproducer and kernel configuration used, which are both too large to
enter into a Git commit.

> >   kernel BUG at fs/ext4/inode.c:2647!
> >   invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> >   CPU: 0 PID: 459 Comm: syz-executor359 Tainted: G        W         5.10.93-syzkaller-01028-g0347b1658399 #0
> 
> What BUG on fs/ext4/inode.c:2647?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/ext4/inode.c?h=v5.10.93#n2647
> 
> All I see is a call to pagevec_release()?  There's a BUG_ON further up
> if we wait for page writeback but then it still has Writeback set.  But
> I don't see anything in pagevec_release that would actually result in a
> BUG warning.

Right, this BUG back-trace was taken from the kernel I received the
bug report for.  I should have used the one I triggered in Mainline,
apologies for that.

The real source of the BUG is in the inlined call to page_buffers().

Here is the link for the latest release kernel:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/fs/ext4/inode.c?h=v5.16#n2620

#define page_buffers(page)                                      \
        ({                                                      \
                BUG_ON(!PagePrivate(page));                     \
                ((struct buffer_head *)page_private(page));     \
        })

> Oh, right, this is one of those inscrutable syzkaller things, where a
> person can spend hours figuring out what the hell it set up.

A link to the config used (again too big to enter into a commit
message), can be easily sourced from the link provided.

> Yeah...no, you don't get to submit patches to core kernel code, claim
> it's not your responsibility to know anything about a subsystem that you
> want to patch, and then expect us to do the work for you.  If you pick
> up a syzkaller report, you get to figure out what broke, why, and how to
> fix it in a reasonable manner.
> 
> You're a maintainer, would you accept a patch like this?

No.  I would share my knowledge to provide a helpful review and work
with the contributor to find a solution (if applicable).

> OH WAIT, you're running this on the Android 5.10 kernel, aren't you?
> The BUG report came from page_buffers failing to find any buffer heads
> attached to the page.
> https://android.googlesource.com/kernel/common/+/refs/heads/android12-5.10-2022-02/fs/ext4/inode.c#2647

Yes, the H/W I have to prototype these on is a phone and the report
that came in was specifically built against the aforementioned
kernel.

> Yeah, don't care.

"There is nothing to worry about, as it's intended behaviour"?

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog