On Sun, 06 Apr 2014 22:28:12 +0100 Ben Hutchings <[email protected]> wrote:
> Here's the backported version I've queued up for 3.2. It's untested; > please let me know if you see any problem with it. > > Ben. > --- > From: Jeff Layton <[email protected]> > Date: Fri, 14 Feb 2014 07:20:35 -0500 > Subject: cifs: ensure that uncached writes handle unmapped areas correctly > > commit 5d81de8e8667da7135d3a32a964087c0faf5483f upstream. > > It's possible for userland to pass down an iovec via writev() that has a > bogus user pointer in it. If that happens and we're doing an uncached > write, then we can end up getting less bytes than we expect from the > call to iov_iter_copy_from_user. This is CVE-2014-0069 > > cifs_iovec_write isn't set up to handle that situation however. It'll > blindly keep chugging through the page array and not filling those pages > with anything useful. Worse yet, we'll later end up with a negative > number in wdata->tailsz, which will confuse the sending routines and > cause an oops at the very least. > > Fix this by having the copy phase of cifs_iovec_write stop copying data > in this situation and send the last write as a short one. At the same > time, we want to avoid sending a zero-length write to the server, so > break out of the loop and set rc to -EFAULT if that happens. This also > allows us to handle the case where no address in the iovec is valid. > > [Note: Marking this for stable on v3.4+ kernels, but kernels as old as > v2.6.38 may have a similar problem and may need similar fix] > > Reviewed-by: Pavel Shilovsky <[email protected]> > Reported-by: Al Viro <[email protected]> > Signed-off-by: Jeff Layton <[email protected]> > Signed-off-by: Steve French <[email protected]> > [bwh: Backported to 3.2: > - Adjust context > - s/nr_pages/npages/ > - s/wdata->pages/pages/ > - In case of an error with no data copied, we must kunmap() page 0, > but in neither case should we free anything else] > Signed-off-by: Ben Hutchings <[email protected]> > --- > fs/cifs/file.c | 37 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 34 insertions(+), 3 deletions(-) > > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -2107,7 +2107,7 @@ cifs_iovec_write(struct file *file, cons > { > unsigned int written; > unsigned long num_pages, npages, i; > - size_t copied, len, cur_len; > + size_t bytes, copied, len, cur_len; > ssize_t total_written = 0; > struct kvec *to_send; > struct page **pages; > @@ -2165,17 +2165,44 @@ cifs_iovec_write(struct file *file, cons > do { > size_t save_len = cur_len; > for (i = 0; i < npages; i++) { > - copied = min_t(const size_t, cur_len, PAGE_CACHE_SIZE); > + bytes = min_t(const size_t, cur_len, PAGE_CACHE_SIZE); > copied = iov_iter_copy_from_user(pages[i], &it, 0, > - copied); > + bytes); > cur_len -= copied; > iov_iter_advance(&it, copied); > to_send[i+1].iov_base = kmap(pages[i]); > to_send[i+1].iov_len = copied; > + /* > + * If we didn't copy as much as we expected, then that > + * may mean we trod into an unmapped area. Stop copying > + * at that point. On the next pass through the big > + * loop, we'll likely end up getting a zero-length > + * write and bailing out of it. > + */ > + if (copied < bytes) > + break; > } > > cur_len = save_len - cur_len; > > + /* > + * If we have no data to send, then that probably means that > + * the copy above failed altogether. That's most likely because > + * the address in the iovec was bogus. Set the rc to -EFAULT, > + * free anything we allocated and bail out. > + */ > + if (!cur_len) { > + kunmap(pages[0]); > + rc = -EFAULT; > + break; > + } > + I don't think this looks quite right in 3.2... If you hit the -EFAULT case above, then you'll break out of the big (outer) do...while loop. The code that handles whether to do a short write or an error code is in that loop, and that break will bypass it. If you didn't actually do any I/O at that point, then cifs_iovec_write is going to return 0 instead of -EFAULT. You'll probably need to rejigger the error handling to make that work properly. Looks like there's also an existing bug in there too if cifs_reopen_file fails... > + /* > + * i + 1 now represents the number of pages we actually used in > + * the copy phase above. > + */ > + npages = i + 1; > + > do { > if (open_file->invalidHandle) { > rc = cifs_reopen_file(open_file, false); > >
signature.asc
Description: PGP signature
