| Message ID | 20191106190400.20969-1-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | iomap: Fix overflow in iomap_page_mkwrite | expand |
On Wed, Nov 06, 2019 at 08:04:00PM +0100, Andreas Gruenbacher wrote: > On architectures where ssize_t is wider than pgoff_t, the expression ssize_t? But you're changing @offset, which is loff_t. I'm confused. Also, which architectures are you talking about here? --D > ((page->index + 1) << PAGE_SHIFT) can overflow. Rewrite to use the page > offset, which we already compute here anyway. > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > fs/iomap/buffered-io.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index e25901ae3ff4..a30ea7ecb790 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1040,20 +1040,19 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops) > > lock_page(page); > size = i_size_read(inode); > - if ((page->mapping != inode->i_mapping) || > - (page_offset(page) > size)) { > + offset = page_offset(page); > + if (page->mapping != inode->i_mapping || offset > size) { > /* We overload EFAULT to mean page got truncated */ > ret = -EFAULT; > goto out_unlock; > } > > /* page is wholly or partially inside EOF */ > - if (((page->index + 1) << PAGE_SHIFT) > size) > + if (offset > size - PAGE_SIZE) > length = offset_in_page(size); > else > length = PAGE_SIZE; > > - offset = page_offset(page); > while (length > 0) { > ret = iomap_apply(inode, offset, length, > IOMAP_WRITE | IOMAP_FAULT, ops, page, > -- > 2.20.1 >
On Wed, Nov 6, 2019 at 8:17 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Wed, Nov 06, 2019 at 08:04:00PM +0100, Andreas Gruenbacher wrote: > > On architectures where ssize_t is wider than pgoff_t, the expression > > ssize_t? But you're changing @offset, which is loff_t. I'm confused. Oops, should have been loff_t instead of ssize_t. > Also, which architectures are you talking about here? From the kernel headers: #define pgoff_t unsigned long typedef long long __kernel_loff_t; typedef __kernel_loff_t loff_t; So for example, sizeof(loff_t) > sizeof(pgoff_t) on x86. Thanks, Andreas
On Wed, Nov 06, 2019 at 08:04:00PM +0100, Andreas Gruenbacher wrote: > On architectures where ssize_t is wider than pgoff_t, the expression > ((page->index + 1) << PAGE_SHIFT) can overflow. Rewrite to use the page > offset, which we already compute here anyway. > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> Looks good except for the ssize_t vs loff_t in the changelog: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Nov 06, 2019 at 08:34:26PM +0100, Andreas Gruenbacher wrote: > On Wed, Nov 6, 2019 at 8:17 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Wed, Nov 06, 2019 at 08:04:00PM +0100, Andreas Gruenbacher wrote: > > > On architectures where ssize_t is wider than pgoff_t, the expression > > > > ssize_t? But you're changing @offset, which is loff_t. I'm confused. > > Oops, should have been loff_t instead of ssize_t. I'll fix it on commit. > > Also, which architectures are you talking about here? > > From the kernel headers: > > #define pgoff_t unsigned long > typedef long long __kernel_loff_t; > typedef __kernel_loff_t loff_t; > > So for example, sizeof(loff_t) > sizeof(pgoff_t) on x86. Ok, that's what I thought. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > Thanks, > Andreas >
On Thu, Nov 7, 2019 at 4:37 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> I'll fix it on commit.
Thanks.
So now the one remaining issue I have with those two functions is why
we check for (offset > size) instead of (offset >= size) in
if (page->mapping != inode->i_mapping || offset > size)
When (offset == size), we're clearly outside the page, and so we should fail?
Thanks,
Andreas
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index e25901ae3ff4..a30ea7ecb790 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1040,20 +1040,19 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops) lock_page(page); size = i_size_read(inode); - if ((page->mapping != inode->i_mapping) || - (page_offset(page) > size)) { + offset = page_offset(page); + if (page->mapping != inode->i_mapping || offset > size) { /* We overload EFAULT to mean page got truncated */ ret = -EFAULT; goto out_unlock; } /* page is wholly or partially inside EOF */ - if (((page->index + 1) << PAGE_SHIFT) > size) + if (offset > size - PAGE_SIZE) length = offset_in_page(size); else length = PAGE_SIZE; - offset = page_offset(page); while (length > 0) { ret = iomap_apply(inode, offset, length, IOMAP_WRITE | IOMAP_FAULT, ops, page,
On architectures where ssize_t is wider than pgoff_t, the expression ((page->index + 1) << PAGE_SHIFT) can overflow. Rewrite to use the page offset, which we already compute here anyway. Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- fs/iomap/buffered-io.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)