From patchwork Fri Nov 6 01:34:02 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Al Viro X-Patchwork-Id: 7565251 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 0DAFD9F4F5 for ; Fri, 6 Nov 2015 01:34:27 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0F84C206DC for ; Fri, 6 Nov 2015 01:34:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 19EB2206AA for ; Fri, 6 Nov 2015 01:34:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031086AbbKFBeJ (ORCPT ); Thu, 5 Nov 2015 20:34:09 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:41124 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965402AbbKFBeI (ORCPT ); Thu, 5 Nov 2015 20:34:08 -0500 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.76 #1 (Red Hat Linux)) id 1ZuVuk-0006N1-Um; Fri, 06 Nov 2015 01:34:02 +0000 Date: Fri, 6 Nov 2015 01:34:02 +0000 From: Al Viro To: Sasha Levin Cc: Andrey Ryabinin , willy@linux.intel.com, Chuck Ebbert , linux-fsdevel , LKML Subject: Re: fs: out of bounds on stack in iov_iter_advance Message-ID: <20151106013402.GT22011@ZenIV.linux.org.uk> References: <55CB5484.6080000@oracle.com> <20150815161338.4ea210ff@as> <55D1A6D4.3080605@gmail.com> <20150819054650.GD18890@ZenIV.linux.org.uk> <55FB75D0.7060403@oracle.com> <560C5469.5010704@oracle.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <560C5469.5010704@oracle.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, 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 On Wed, Sep 30, 2015 at 05:30:17PM -0400, Sasha Levin wrote: > > So I've traced this all the way back to dax_io(). I can trigger this with: > > > > diff --git a/fs/dax.c b/fs/dax.c > > index 93bf2f9..2cdb8a5 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -178,6 +178,7 @@ static ssize_t dax_io(struct inode *inode, struct iov_iter *iter, > > if (need_wmb) > > wmb_pmem(); > > > > + WARN_ON((pos == start) && (pos - start > iov_iter_count(iter))); > > return (pos == start) ? retval : pos - start; > > } > > > > So it seems that iter gets moved twice here: once in dax_io(), and once again > > back at generic_file_read_iter(). > > > > I don't see how it ever worked. Am I missing something? This: struct iov_iter data = *iter; retval = mapping->a_ops->direct_IO(iocb, &data, pos); } if (retval > 0) { *ppos = pos + retval; iov_iter_advance(iter, retval); The iterator advanced in ->direct_IO() is a _copy_, not the original. The contents of *iter as seen by generic_file_read_iter() is not modifiable by ->direct_IO(), simply because its address is nowhere to be found. And checking iov_iter_count(iter) at the end of dax_io() is pointless - from the POV of generic_file_read_iter() it's data.count, and while it used to be equal to iter->count, it's already been modified. By the time we call iov_iter_advance() in generic_file_read_iter() that value will be already discarded, along with rest of struct iov_iter data. Wait a minute - you are triggering _what_??? > > + WARN_ON((pos == start) && (pos - start > iov_iter_count(iter))); With '&&'? iov_iter_count() is size_t, while pos and start are loff_t, so you are seeing equal values in pos and start (as integers) *and* (loff_t)0 > (size_t)something. loff_t is a signed type, size_t - unsigned. 6.3.1.8[1] says that * if rank of size_t is greater or equal to rank of loff_t, the latter gets converted to size_t. And conversion of zero should be zero, i.e. (size_t) 0 > (size_t)something, which is impossible (we compare them as non-negative integers). * if loff_t can represent all values of size_t, size_t value gets converted to loff_t. Result of conversion should have the same (in particular, non-negative) value. Again, comparison can't be true. * otherwise both values are converted to unsigned counterpart of loff_t. Again, zero converts to 0 and in any unsigned type 0 > x is impossible. I don't see any way for that condition to evaluate true. Assuming that it's a misquoted ||... I don't see any way for pos to get greater than start + original iov_iter_count(). However, I *do* see a way for bad things to happen in a different way. Look: // first pass through the loop, pos == start (and so's max) retval = dax_get_addr(bh, &addr, blkbits); // got a positive value if (retval < 0) break; // nope, keep going if (buffer_unwritten(bh) || buffer_new(bh)) { dax_new_buf(addr, retval, first, pos, end); need_wmb = true; } addr += first; size = retval - first; // OK... } max = min(pos + size, end); // OK... } if (iov_iter_rw(iter) == WRITE) { len = copy_from_iter_pmem(addr, max - pos, iter); need_wmb = true; } else if (!hole) len = copy_to_iter((void __force *)addr, max - pos, iter); else len = iov_iter_zero(max - pos, iter); // too bad - we'd hit an unmapped memory area. len is 0... // and retval is fucking positive. if (!len) break; return (pos == start) ? retval : pos - start; // will return a bloody big positive value Could you try to reproduce it with this: dax_io(): don't let non-error value escape via retval instead of EFAULT Signed-off-by: Al Viro Reported-by: Sasha Levin --- -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/dax.c b/fs/dax.c index a86d3cc..7b653e9 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -169,8 +169,10 @@ static ssize_t dax_io(struct inode *inode, struct iov_iter *iter, else len = iov_iter_zero(max - pos, iter); - if (!len) + if (!len) { + retval = -EFAULT; break; + } pos += len; addr += len;