Re: [syzbot] BUG: unable to handle kernel paging request in truncate_inode_partial_folio

[Mike Rapoport]

On Thu, Jun 30, 2022 at 09:14:07AM -0700, Axel Rasmussen wrote:
> On Thu, Jun 30, 2022 at 1:47 AM Mike Rapoport <rppt@xxxxxxxxxx> wrote:
> > On Wed, Jun 29, 2022 at 09:30:12AM -0700, Axel Rasmussen wrote:
> > > On Tue, Jun 28, 2022 at 9:41 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> > > > On Tue, Jun 28, 2022 at 03:59:26PM -0700, syzbot wrote:
> > > > > Hello,
> > > > >
> > > > > syzbot found the following issue on:
> > > > >
> > > > > HEAD commit:    941e3e791269 Merge tag 'for_linus' of git://git.kernel.org..
> > > > > git tree:       upstream
> > > > > console+strace: https://syzkaller.appspot.com/x/log.txt?x=1670ded4080000
> > > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=833001d0819ddbc9
> > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=9bd2b7adbd34b30b87e4
> > > > > compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> > > > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=140f9ba8080000
> > > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=15495188080000
> > > > >
> > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > Reported-by: syzbot+9bd2b7adbd34b30b87e4@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > >
> > > >
> > > > I think this is a bug in memfd_secret.  secretmem_setattr() can race with a page
> > > > being faulted in by secretmem_fault().  Specifically, a page can be faulted in
> > > > after secretmem_setattr() has set i_size but before it zeroes out the partial
> > > > page past i_size.  memfd_secret pages aren't mapped in the kernel direct map, so
> > > > the crash occurs when the kernel tries to zero out the partial page.
> > > >
> > > > I don't know what the best solution is -- maybe a rw_semaphore protecting
> > > > secretmem_fault() and secretmem_setattr()?  Or perhaps secretmem_setattr()
> > > > should avoid the call to truncate_setsize() by not using simple_setattr(), given
> > > > that secretmem_setattr() only supports the size going from zero to nonzero.
> > >
> > > From my perspective the rw_semaphore approach sounds reasonable.
> > >
> > > simple_setattr() and the functions it calls to do the actual work
> > > isn't a tiny amount of code, it would be a shame to reimplement it in
> > > secretmem.c.
> > >
> > > For the rwsem, I guess the idea is setattr will take it for write, and
> > > fault will take it for read? Since setattr is a very infrequent
> > > operation - a typical use case is you'd do it exactly once right after
> > > opening the memfd_secret - this seems like it wouldn't make fault
> > > significantly less performant. It's also a pretty small change I
> > > think, just a few lines.
> >
> > Below is my take on adding a semaphore and making ->setattr() and ->fault()
> > mutually exclusive. It's only lightly tested so I'd appreciate if Eric
> > could give it a whirl.
> >
> > With addition of semaphore to secretmem_setattr() it seems we don't need
> > special care for size changes, just calling simple_setattr() after taking
> > the semaphore should be fine. Thoughts?
> 
> The patch below looks correct to me. I do think we still need the
> check which prevents truncating a memfd_secret with an existing
> nonzero size, though, because I think simple_setattr's way of doing
> that still BUGs in a non-racy way (rwsem doesn't help with this). The
> patch below keeps this, so maybe I'm just misinterpreting "we don't
> need special care for size changes".

It really was a question, because I was too lazy to dig into
simple_setattr() and I know you investigated it :)
 
> I haven't booted+tested it, I'll leave that to Eric since he already
> has a reproducer setup for this. But, for what it's worth, feel free
> to take:
> 
> Reviewed-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>

Thanks!

-- 
Sincerely yours,
Mike.