From patchwork Sun Apr 6 21:28:12 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Hutchings X-Patchwork-Id: 3943771 Return-Path: X-Original-To: patchwork-cifs-client@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 423309F374 for ; Sun, 6 Apr 2014 21:28:34 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 71B4120259 for ; Sun, 6 Apr 2014 21:28:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B63352024C for ; Sun, 6 Apr 2014 21:28:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752575AbaDFV2b (ORCPT ); Sun, 6 Apr 2014 17:28:31 -0400 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:59178 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750797AbaDFV2a (ORCPT ); Sun, 6 Apr 2014 17:28:30 -0400 Received: from deadeye.i.decadent.org.uk ([192.168.2.102] helo=deadeye.wl.decadent.org.uk) by shadbolt.decadent.org.uk with esmtps (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1WWuc2-0002VI-4W; Sun, 06 Apr 2014 22:28:22 +0100 Received: from ben by deadeye.wl.decadent.org.uk with local (Exim 4.82) (envelope-from ) id 1WWuc1-0007K3-Gn; Sun, 06 Apr 2014 22:28:21 +0100 Message-ID: <1396819692.13361.4.camel@deadeye.wl.decadent.org.uk> Subject: [PATCH 3.2] cifs: ensure that uncached writes handle unmapped areas correctly From: Ben Hutchings To: Jeff Layton Cc: Pavel Shilovsky , Steve French , stable , linux-cifs@vger.kernel.org Date: Sun, 06 Apr 2014 22:28:12 +0100 X-Mailer: Evolution 3.8.5-2+b3 Mime-Version: 1.0 X-SA-Exim-Connect-IP: 192.168.2.102 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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 Reported-by: Al Viro Signed-off-by: Jeff Layton Signed-off-by: Steve French [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 --- 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 + 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);