On Sat, Jul 07 2018, Jann Horn wrote: >> @@ -287,11 +278,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) >> goto Efault; >> copied += n; >> m->count -= n; >> - if (m->count) >> - m->from = n; >> - else >> - pos++; >> - m->index = pos; >> + m->from = n; > > This patch introduces a kernel memory disclosure bug when something > like the following sequence of events happens (starting from a freshly > opened seq file): > > 1. read(seq_fd, buf, 2000): sets m->from=2000, m->count=100 > 2. create a buffer broken_buf which consists of 1000 bytes writable > memory followed by unmapped memory > 3. read(seq_fd, broken_buf, 3100): > - flushes buffered data to userspace, result: m->from=2100, m->count=0 > - accumulates new data, result: m->from=2100, m->count=3050 > - tries to copy new data to userspace, but fails ("goto Efault") > 4. read(seq_fd, buf, 4096): does copy_to_user(buf, m->buf + m->from, n) Thanks for testing and for the report. I think I see where I went wrong in the patch. As I said in the description: - don't clear ->from when ->count is zero, as ->from is dead when ->count is zero. It is true that ->from is dead when ->count is zero, but as soon as count becomes non-zero, ->from becomes important again. So we either need to clear ->from whenever ->count is changed from zero (which would be clumsy and error prone) we we need to clear ->from somewhere else. ->count is only increased in ->show() calls and there are three ->show() calls. - in traverse() ->from is set to zero early, and set once more shortly before the function exits, so it is always correct. - in "we need at least one record in buffer" ->count starts at zero so ->from needs to be set to zero as well. - in "Fill:" ->from is still correct from previous setting. So I think we just need m->from = 0; at "we need at least one record in buffer". I'm fairly sure that will fix the problem you found. I would appreciate it if you would test and confirm. I'll send a patch separately. Thanks again, NeilBrown