* [PATCH] iomap: Fix overflow in iomap_page_mkwrite
@ 2019-11-06 19:04 Andreas Gruenbacher
2019-11-06 19:16 ` Darrick J. Wong
2019-11-07 8:44 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Andreas Gruenbacher @ 2019-11-06 19:04 UTC (permalink / raw)
To: linux-xfs
Cc: Alexander Viro, linux-fsdevel, Christoph Hellwig, Andreas Gruenbacher
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iomap: Fix overflow in iomap_page_mkwrite
2019-11-06 19:04 [PATCH] iomap: Fix overflow in iomap_page_mkwrite Andreas Gruenbacher
@ 2019-11-06 19:16 ` Darrick J. Wong
2019-11-06 19:34 ` Andreas Gruenbacher
2019-11-07 8:44 ` Christoph Hellwig
1 sibling, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2019-11-06 19:16 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: linux-xfs, Alexander Viro, linux-fsdevel, Christoph Hellwig
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iomap: Fix overflow in iomap_page_mkwrite
2019-11-06 19:16 ` Darrick J. Wong
@ 2019-11-06 19:34 ` Andreas Gruenbacher
2019-11-07 15:37 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Gruenbacher @ 2019-11-06 19:34 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-xfs, Alexander Viro, linux-fsdevel, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iomap: Fix overflow in iomap_page_mkwrite
2019-11-06 19:34 ` Andreas Gruenbacher
@ 2019-11-07 15:37 ` Darrick J. Wong
2019-11-07 17:54 ` Andreas Gruenbacher
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2019-11-07 15:37 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: linux-xfs, Alexander Viro, linux-fsdevel, Christoph Hellwig
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iomap: Fix overflow in iomap_page_mkwrite
2019-11-07 15:37 ` Darrick J. Wong
@ 2019-11-07 17:54 ` Andreas Gruenbacher
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Gruenbacher @ 2019-11-07 17:54 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-xfs, Alexander Viro, linux-fsdevel, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iomap: Fix overflow in iomap_page_mkwrite
2019-11-06 19:04 [PATCH] iomap: Fix overflow in iomap_page_mkwrite Andreas Gruenbacher
2019-11-06 19:16 ` Darrick J. Wong
@ 2019-11-07 8:44 ` Christoph Hellwig
1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2019-11-07 8:44 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: linux-xfs, Alexander Viro, linux-fsdevel, Christoph Hellwig
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-07 17:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06 19:04 [PATCH] iomap: Fix overflow in iomap_page_mkwrite Andreas Gruenbacher
2019-11-06 19:16 ` Darrick J. Wong
2019-11-06 19:34 ` Andreas Gruenbacher
2019-11-07 15:37 ` Darrick J. Wong
2019-11-07 17:54 ` Andreas Gruenbacher
2019-11-07 8:44 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).