| Message ID | 20180907191624.186623-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs: require i_size <= SIZE_MAX in kernel_read_file() | expand |
On Fri, 2018-09-07 at 12:16 -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > On 32-bit systems, the buffer allocated by kernel_read_file() is too > small if the file size is > SIZE_MAX, due to truncation to size_t. > > Fortunately, since the 'count' argument to kernel_read() is also > truncated to size_t, only the allocated space is filled; then, -EIO is > returned since 'pos != i_size' after the read loop. > > But this is not obvious and seems incidental. We should be more > explicit about this case. So, fail early if i_size > SIZE_MAX. > > Signed-off-by: Eric Biggers <ebiggers@google.com> Thanks! > --- > fs/exec.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 1ebf6e5a521d..fc281b738a98 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -908,14 +908,14 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > goto out; > > i_size = i_size_read(file_inode(file)); > - if (max_size > 0 && i_size > max_size) { > - ret = -EFBIG; > - goto out; > - } > if (i_size <= 0) { > ret = -EINVAL; > goto out; > } > + if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { > + ret = -EFBIG; > + goto out; > + } > > if (id != READING_FIRMWARE_PREALLOC_BUFFER) > *buf = vmalloc(i_size);
On Fri, Sep 07, 2018 at 12:16:24PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > On 32-bit systems, the buffer allocated by kernel_read_file() is too > small if the file size is > SIZE_MAX, due to truncation to size_t. > > Fortunately, since the 'count' argument to kernel_read() is also > truncated to size_t, only the allocated space is filled; then, -EIO is > returned since 'pos != i_size' after the read loop. > > But this is not obvious and seems incidental. We should be more > explicit about this case. So, fail early if i_size > SIZE_MAX. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > fs/exec.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 1ebf6e5a521d..fc281b738a98 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -908,14 +908,14 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > goto out; > > i_size = i_size_read(file_inode(file)); > - if (max_size > 0 && i_size > max_size) { > - ret = -EFBIG; > - goto out; > - } > if (i_size <= 0) { > ret = -EINVAL; > goto out; > } > + if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { > + ret = -EFBIG; > + goto out; > + } > > if (id != READING_FIRMWARE_PREALLOC_BUFFER) > *buf = vmalloc(i_size); > -- Al, are you planning to apply this? - Eric
diff --git a/fs/exec.c b/fs/exec.c index 1ebf6e5a521d..fc281b738a98 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -908,14 +908,14 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, goto out; i_size = i_size_read(file_inode(file)); - if (max_size > 0 && i_size > max_size) { - ret = -EFBIG; - goto out; - } if (i_size <= 0) { ret = -EINVAL; goto out; } + if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { + ret = -EFBIG; + goto out; + } if (id != READING_FIRMWARE_PREALLOC_BUFFER) *buf = vmalloc(i_size);