From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f67.google.com ([209.85.218.67]:32835 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932856AbeGITlW (ORCPT ); Mon, 9 Jul 2018 15:41:22 -0400 Received: by mail-oi0-f67.google.com with SMTP id c6-v6so38157538oiy.0 for ; Mon, 09 Jul 2018 12:41:22 -0700 (PDT) MIME-Version: 1.0 References: <87vacsrt0r.fsf@notabene.neil.brown.name> <87fu3dihtf.fsf@notabene.neil.brown.name> <874lintqa6.fsf@notabene.neil.brown.name> <87y3fcegnn.fsf@notabene.neil.brown.name> <878t6nybj7.fsf@notabene.neil.brown.name> <87601ryb8a.fsf@notabene.neil.brown.name> In-Reply-To: From: Jann Horn Date: Mon, 9 Jul 2018 21:40:54 +0200 Message-ID: Subject: Re: [PATCH mm] VFS: seq_file: ensure ->from is valid. To: Kees Cook Cc: neilb@suse.com, Andrew Morton , Al Viro , Linus Torvalds , linux-doc@vger.kernel.org, kernel list , linux-fsdevel@vger.kernel.org, Jonathan Corbet Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Jul 9, 2018 at 8:16 PM Kees Cook wrote: > > On Fri, Jul 6, 2018 at 8:29 PM, NeilBrown wrote: > > > > Previous patch ("VFS: simplify seq_file iteration code and interface") > > removed code to set ->from to zero when ->count is zero, as ->from is > > dead at that time. However it didn't ensure ->from was set properly > > whenever ->count becomes non-zero. > > This can only happen when ->show() is called. Of the three places it > > is called one already has ->from set to zero. The other two are > > fixed by setting from to zero after fully flushing the buffer (at which > > point ->count will also be zero). > > > > Reported-by: Jann Horn > > Signed-off-by: NeilBrown > > I *think* this solves this report, which looks very much like Jann's reproducer: > > https://syzkaller.appspot.com/bug?extid=4b712dce5cbce6700f27 I don't think the path I found and what syzcaller reported match precisely. The syz reproducer linked on that page is: r0 = memfd_create(&(0x7f00000000c0)='md5sum\x00', 0x0) mmap(&(0x7f0000001000/0x1000)=nil, 0x1000, 0x0, 0x51, r0, 0x0) mkdir(&(0x7f0000000040)='./control\x00', 0x0) r1 = socket$inet_sctp(0x2, 0x1, 0x84) getsockopt$inet_sctp_SCTP_ADAPTATION_LAYER(r1, 0x84, 0x7, &(0x7f0000000000), &(0x7f0000000080)=0x4) r2 = syz_open_procfs(0x0, &(0x7f0000000040)='smaps\x00') readv(r2, &(0x7f00000021c0)=[{&(0x7f0000000140)=""/79, 0x432}, {&(0x7f00000001c0)=""/4096, 0x1000}], 0x2) If we assume that this is the same bug, only the last two lines are interesting. They should cause two invocations of seq_read(). The first invocation should start with ->from==0, so the bug shouldn't trigger there. So it has to happen on the second invocation. So the first invocation has to leave ->from at some high value and ->count nonzero. But the only place in seq_read() that could set ->from on the first read (if the traverse case doesn't happen) is the one at the end of seq_read() that properly adjusts ->count down by the same number in the line above. This doesn't add up.