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.
