On Sun, 06 Apr 2014 22:28:12 +0100
Ben Hutchings <ben@xxxxxxxxxxxxxxx> 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 <jlayton@xxxxxxxxxx>
> 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 <piastry@xxxxxxxxxxx>
> Reported-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Signed-off-by: Steve French <smfrench@xxxxxxxxx>
> [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 <ben@xxxxxxxxxxxxxxx>
> ---
> 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);
>
>
Attachment:
signature.asc
Description: PGP signature