lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <1403395400.2592.4.camel@jarvis.lan> Date: Sat, 21 Jun 2014 17:03:20 -0700 From: James Bottomley <James.Bottomley@...senPartnership.com> To: Al Viro <viro@...IV.linux.org.uk> Cc: Theodore Ts'o <tytso@....edu>, Dave Chinner <david@...morbit.com>, Jens Axboe <axboe@...com>, linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org Subject: Re: 32-bit bug in iovec iterator changes On Sun, 2014-06-22 at 00:49 +0100, Al Viro wrote: > On Sat, Jun 21, 2014 at 07:09:22PM -0400, Theodore Ts'o wrote: > > On Sat, Jun 21, 2014 at 06:53:07AM +0100, Al Viro wrote: > > > > > > ed include/linux/uio.h <<EOF > > > /iov_iter_truncate/s/size_t/u64/ > > > w > > > q > > > EOF > > > > > > Could you check if that fixes the sucker? > > > > The following patch (attached at the end) appears to fix the problem, > > but looking at uio.h, I'm completely confused about *why* it fixes the > > problem. In particular, iov_iter_iovec() makes no sense to me at all, > > and I don't understand how the calculation of iov_len makes any sense: > > > > .iov_len = min(iter->count, > > iter->iov->iov_len - iter->iov_offset), > > Eh? We have iov[0].iov_base..iov[0].iov_base+iov[0].iov_len - 1 for > area covered by the first iovec. First iov_offset bytes have already > been consumed. And at most count bytes matter. So yes, this iov_len > will give you equivalent first iovec. > > Said that, iov_iter_iovec() will die shortly - it's a rudiment of older > code, with almost no users left. But yes, it is correct. > > > It also looks like uio.h is mostly about offsets to memory pointers, > > and so why this would make a difference when the issue is the block > > device offset goes above 2**30? > > It is, and your patch is a huge overkill. > > > There must be deep magic going on here, and so I don't know if your > > s/size_t/u64/g substitation also extends to the various functions that > > have size_t in them: > > No, it does not. It's specifically about iov_iter_truncate(); moreover, > it matters to only one caller of that sucker. Namely, > > static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to) > { > struct file *file = iocb->ki_filp; > struct inode *bd_inode = file->f_mapping->host; > loff_t size = i_size_read(bd_inode); > loff_t pos = iocb->ki_pos; > > if (pos >= size) > return 0; > > size -= pos; > iov_iter_truncate(to, size); > return generic_file_read_iter(iocb, to); > } > > What happens here is capping to->count, to guarantee that we won't even look > at anything past the end of block device. Alternative fix would be to > have > if (pos >= size) > return 0; > if (to->size + pos > size) { > /* note that size - pos fits into size_t in this case, > * so it's OK to pass it to iov_iter_truncate(). > */ > iov_iter_truncate(to, size - pos); > } > return generic_file_read_iter(iocb, to); > in there. Other callers are passing it size_t values already, so we don't > need similar checks there. > > Or we can make iov_iter_truncate() take an arbitrary u64 argument, seeing that > it's inlined anyway. IMO it's more robust that way... > > Anyway, does the following alone fix the problem you are seeing? > > diff --git a/include/linux/uio.h b/include/linux/uio.h > index ddfdb53..dbb02d4 100644 > --- a/include/linux/uio.h > +++ b/include/linux/uio.h > @@ -94,7 +94,7 @@ static inline size_t iov_iter_count(struct iov_iter *i) > return i->count; > } > > -static inline void iov_iter_truncate(struct iov_iter *i, size_t count) > +static inline void iov_iter_truncate(struct iov_iter *i, u64 count) > { > if (i->count > count) > i->count = count; Al, how can that work? i->count is size_t, which is 32 bit, so we're going to get truncation errors. I could see this possibly working if count in struct iov_iter becomes u64 (which is going to have a lot of knock on consequences, but it seems to me that at least kvec.iov_len and iov_iter.iov_offset have to become u64 as well. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists