| Message ID | 0e7def98-1479-4f3a-a69a-5f4d09e12fa8@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: check for negatives in xfs_exchange_range_checks() | expand |
…
> Check the they aren't negative.
Would you like to use the word “that” (instead of “the”) in this sentence?
Regards,
Markus
On Sat, May 04, 2024 at 02:27:36PM +0300, Dan Carpenter wrote: > The fxr->file1_offset and fxr->file2_offset variables come from the user > in xfs_ioc_exchange_range(). They are size loff_t which is an s64. > Check the they aren't negative. > > Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") In this commit file1_offset and file2_offset are u64. They used to be u64 in the initial submission, but we changed that as part of the review process.
On Mon, May 06, 2024 at 11:06:17PM -0700, Christoph Hellwig wrote: > On Sat, May 04, 2024 at 02:27:36PM +0300, Dan Carpenter wrote: > > The fxr->file1_offset and fxr->file2_offset variables come from the user > > in xfs_ioc_exchange_range(). They are size loff_t which is an s64. > > Check the they aren't negative. > > > > Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") > > In this commit file1_offset and file2_offset are u64. They used to > be u64 in the initial submission, but we changed that as part of the > review process. I've just checked again, and I think it was loff_t in that commit. There are two related structs, the one that's userspace API and the one that's internal. The userspace API is u64 but internally it's loff_t. fs/xfs/libxfs/xfs_fs.h 818 struct xfs_exchange_range { 819 __s32 file1_fd; 820 __u32 pad; /* must be zeroes */ 821 __u64 file1_offset; /* file1 offset, bytes */ 822 __u64 file2_offset; /* file2 offset, bytes */ 823 __u64 length; /* bytes to exchange */ 824 825 __u64 flags; /* see XFS_EXCHANGE_RANGE_* below */ 826 }; fs/xfs/xfs_exchrange.h 16 struct xfs_exchrange { 17 struct file *file1; 18 struct file *file2; 19 20 loff_t file1_offset; 21 loff_t file2_offset; 22 u64 length; 23 24 u64 flags; /* XFS_EXCHANGE_RANGE flags */ 25 }; regards, dan carpenter
On Tue, May 07, 2024 at 09:33:40AM +0300, Dan Carpenter wrote: > On Mon, May 06, 2024 at 11:06:17PM -0700, Christoph Hellwig wrote: > > On Sat, May 04, 2024 at 02:27:36PM +0300, Dan Carpenter wrote: > > > The fxr->file1_offset and fxr->file2_offset variables come from the user > > > in xfs_ioc_exchange_range(). They are size loff_t which is an s64. > > > Check the they aren't negative. > > > > > > Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") > > > > In this commit file1_offset and file2_offset are u64. They used to > > be u64 in the initial submission, but we changed that as part of the > > review process. > > I've just checked again, and I think it was loff_t in that commit. > There are two related structs, the one that's userspace API and the > one that's internal. The userspace API is u64 but internally it's > loff_t. Ah, yes. The in-kernel ones probably just needs to move to use u64 as well.
On Sat, May 04, 2024 at 02:27:36PM +0300, Dan Carpenter wrote: > The fxr->file1_offset and fxr->file2_offset variables come from the user > in xfs_ioc_exchange_range(). They are size loff_t which is an s64. > Check the they aren't negative. > > Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > From static analysis. Untested. Sorry! Not a fan of this ^^^^^^^^ > > fs/xfs/xfs_exchrange.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/xfs/xfs_exchrange.c b/fs/xfs/xfs_exchrange.c > index c8a655c92c92..3465e152d928 100644 > --- a/fs/xfs/xfs_exchrange.c > +++ b/fs/xfs/xfs_exchrange.c > @@ -337,6 +337,9 @@ xfs_exchange_range_checks( > if (IS_SWAPFILE(inode1) || IS_SWAPFILE(inode2)) > return -ETXTBSY; > > + if (fxr->file1_offset < 0 || fxr->file2_offset < 0) > + return -EINVAL; but this looks right to me. If you actually test your changes, then Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + > size1 = i_size_read(inode1); > size2 = i_size_read(inode2); > > -- > 2.43.0 > >
On Mon, May 06, 2024 at 11:40:25PM -0700, Christoph Hellwig wrote: > On Tue, May 07, 2024 at 09:33:40AM +0300, Dan Carpenter wrote: > > On Mon, May 06, 2024 at 11:06:17PM -0700, Christoph Hellwig wrote: > > > On Sat, May 04, 2024 at 02:27:36PM +0300, Dan Carpenter wrote: > > > > The fxr->file1_offset and fxr->file2_offset variables come from the user > > > > in xfs_ioc_exchange_range(). They are size loff_t which is an s64. > > > > Check the they aren't negative. > > > > > > > > Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") > > > > > > In this commit file1_offset and file2_offset are u64. They used to > > > be u64 in the initial submission, but we changed that as part of the > > > review process. > > > > I've just checked again, and I think it was loff_t in that commit. > > There are two related structs, the one that's userspace API and the > > one that's internal. The userspace API is u64 but internally it's > > loff_t. > > Ah, yes. The in-kernel ones probably just needs to move to use u64 > as well. I don't think we want userspace to be able to exchangerange data at file positions that they can't read or write with a standard fs syscall. --D
On Sat, May 04, 2024 at 02:27:36PM +0300, Dan Carpenter wrote: > The fxr->file1_offset and fxr->file2_offset variables come from the user > in xfs_ioc_exchange_range(). They are size loff_t which is an s64. > Check the they aren't negative. > > Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > From static analysis. Untested. Sorry! > > fs/xfs/xfs_exchrange.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/xfs/xfs_exchrange.c b/fs/xfs/xfs_exchrange.c > index c8a655c92c92..3465e152d928 100644 > --- a/fs/xfs/xfs_exchrange.c > +++ b/fs/xfs/xfs_exchrange.c > @@ -337,6 +337,9 @@ xfs_exchange_range_checks( > if (IS_SWAPFILE(inode1) || IS_SWAPFILE(inode2)) > return -ETXTBSY; > > + if (fxr->file1_offset < 0 || fxr->file2_offset < 0) > + return -EINVAL; Aren't the operational offset/lengths already checked for underflow and overflow via xfs_exchange_range_verify_area()? -Dave.
On Wed, May 08, 2024 at 11:29:15AM +1000, Dave Chinner wrote: > On Sat, May 04, 2024 at 02:27:36PM +0300, Dan Carpenter wrote: > > The fxr->file1_offset and fxr->file2_offset variables come from the user > > in xfs_ioc_exchange_range(). They are size loff_t which is an s64. > > Check the they aren't negative. > > > > Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > From static analysis. Untested. Sorry! > > > > fs/xfs/xfs_exchrange.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/xfs/xfs_exchrange.c b/fs/xfs/xfs_exchrange.c > > index c8a655c92c92..3465e152d928 100644 > > --- a/fs/xfs/xfs_exchrange.c > > +++ b/fs/xfs/xfs_exchrange.c > > @@ -337,6 +337,9 @@ xfs_exchange_range_checks( > > if (IS_SWAPFILE(inode1) || IS_SWAPFILE(inode2)) > > return -ETXTBSY; > > > > + if (fxr->file1_offset < 0 || fxr->file2_offset < 0) > > + return -EINVAL; > > Aren't the operational offset/lengths already checked for underflow > and overflow via xfs_exchange_range_verify_area()? Oh, yeah, they are. I was just thinking surely I wrote some tests to pass in garbage offsets and bounce back out... --D > -Dave. > -- > Dave Chinner > david@fromorbit.com
On Wed, May 08, 2024 at 11:29:15AM +1000, Dave Chinner wrote: > On Sat, May 04, 2024 at 02:27:36PM +0300, Dan Carpenter wrote: > > The fxr->file1_offset and fxr->file2_offset variables come from the user > > in xfs_ioc_exchange_range(). They are size loff_t which is an s64. > > Check the they aren't negative. > > > > Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > From static analysis. Untested. Sorry! > > > > fs/xfs/xfs_exchrange.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/xfs/xfs_exchrange.c b/fs/xfs/xfs_exchrange.c > > index c8a655c92c92..3465e152d928 100644 > > --- a/fs/xfs/xfs_exchrange.c > > +++ b/fs/xfs/xfs_exchrange.c > > @@ -337,6 +337,9 @@ xfs_exchange_range_checks( > > if (IS_SWAPFILE(inode1) || IS_SWAPFILE(inode2)) > > return -ETXTBSY; > > > > + if (fxr->file1_offset < 0 || fxr->file2_offset < 0) > > + return -EINVAL; > > Aren't the operational offset/lengths already checked for underflow > and overflow via xfs_exchange_range_verify_area()? Ah right. Smatch complains in the middle of the two calls to xfs_exchange_range_verify_area(). (It get's called in different places depending on if the XFS_EXCHANGE_RANGE_TO_EOF flag is set). regards, dan carpenter
diff --git a/fs/xfs/xfs_exchrange.c b/fs/xfs/xfs_exchrange.c index c8a655c92c92..3465e152d928 100644 --- a/fs/xfs/xfs_exchrange.c +++ b/fs/xfs/xfs_exchrange.c @@ -337,6 +337,9 @@ xfs_exchange_range_checks( if (IS_SWAPFILE(inode1) || IS_SWAPFILE(inode2)) return -ETXTBSY; + if (fxr->file1_offset < 0 || fxr->file2_offset < 0) + return -EINVAL; + size1 = i_size_read(inode1); size2 = i_size_read(inode2);
The fxr->file1_offset and fxr->file2_offset variables come from the user in xfs_ioc_exchange_range(). They are size loff_t which is an s64. Check the they aren't negative. Fixes: 9a64d9b3109d ("xfs: introduce new file range exchange ioctl") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- From static analysis. Untested. Sorry! fs/xfs/xfs_exchrange.c | 3 +++ 1 file changed, 3 insertions(+)