| Message ID | 87601ryb8a.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 7, 2018 at 5:29 AM NeilBrown <neilb@suse.com> 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 <jannh@google.com> > Signed-off-by: NeilBrown <neilb@suse.com> Tested-by: Jann Horn <jannh@google.com> > --- > fs/seq_file.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/seq_file.c b/fs/seq_file.c > index fd82585ab50f..1dea7a8a5255 100644 > --- a/fs/seq_file.c > +++ b/fs/seq_file.c > @@ -220,6 +220,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) > goto Done; > } > /* we need at least one record in buffer */ > + m->from = 0; > p = m->op->start(m, &m->index); > while (1) { > err = PTR_ERR(p); This looks correct to me. I have also tested that with this patch applied, my crasher doesn't work anymore.
On Fri, Jul 6, 2018 at 8:29 PM, NeilBrown <neilb@suse.com> 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 <jannh@google.com> > Signed-off-by: NeilBrown <neilb@suse.com> I *think* this solves this report, which looks very much like Jann's reproducer: https://syzkaller.appspot.com/bug?extid=4b712dce5cbce6700f27 -Kees
On Mon, Jul 9, 2018 at 8:16 PM Kees Cook <keescook@chromium.org> wrote: > > On Fri, Jul 6, 2018 at 8:29 PM, NeilBrown <neilb@suse.com> 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 <jannh@google.com> > > Signed-off-by: NeilBrown <neilb@suse.com> > > 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.
diff --git a/fs/seq_file.c b/fs/seq_file.c index fd82585ab50f..1dea7a8a5255 100644 --- a/fs/seq_file.c +++ b/fs/seq_file.c @@ -220,6 +220,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) goto Done; } /* we need at least one record in buffer */ + m->from = 0; p = m->op->start(m, &m->index); while (1) { err = PTR_ERR(p);
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 <jannh@google.com> Signed-off-by: NeilBrown <neilb@suse.com> --- fs/seq_file.c | 1 + 1 file changed, 1 insertion(+)