On Mon, 2014-03-17 at 17:30 +0100, Raphael Geissert wrote:
> Source: linux
> Version: 3.2.51-1
> Tags: patch security
> X-debbugs-cc: jmm@debian.org
>
> Hi,
>
> Attached patch is what I believe would be the correct backport for 3.2
> of the specific fix for CVE-2014-0069, which is
> 5d81de8e8667da7135d3a32a964087c0faf5483f.
Sorry, I forgot about this and ended up doing my own backport
(attached). Comparing my diff with yours:
[...]
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index c55808e..fd07d24 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2107,7 +2107,7 @@ cifs_iovec_write(struct file *file, const struct iovec *iov,
> {
> 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,45 @@ cifs_iovec_write(struct file *file, const struct iovec *iov,
> 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;
> }
All the same so far.
> + /*
> + * i + 1 now represents the number of pages we actually used in
> + * the copy phase above. Bring npages down to that.
> + */
> + for ( ; npages > i + 1; npages--) ;
> +
I simplified this to:
npages = i + 1;
but the comment and that simplification are only correct in the error
case so I should use min().
> 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,
> + * and bail out.
> + */
> + if (!cur_len) {
> + for (i = 0; i < npages; i++)
> + kunmap(pages[i]);
If cur_len == 0 then also i == 0 and npages == 1, so this loop is
equivalent to:
kunmap(pages[0]);
which is what I used (avoiding the need to reorder code).
> + total_written = -EFAULT;
Good spot, I left this as rc = -EFAULT which will have no effect.
However, to match the upstream version, this assignment should be
dependent on !total_written (i.e. the failure happened before anything
was written).
I'll send another mail with my revised version.
Ben.
> + break;
> + }
> +
> do {
> if (open_file->invalidHandle) {
> rc = cifs_reopen_file(open_file, false);
--
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.
Attachment:
signature.asc
Description: This is a digitally signed message part