[PATCH] vfs: prevent signed overflow by using u64 over loff_t

* [PATCH] vfs: prevent signed overflow by using u64 over loff_t
@ 2020-01-13  6:40 Saagar Jha
  2020-01-14  7:54 ` kbuild test robot
  2020-01-16 15:21 ` [PATCH] " kbuild test robot
  0 siblings, 2 replies; 5+ messages in thread
From: Saagar Jha @ 2020-01-13  6:40 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: viro

Hi,

I think I’ve found a signed integer overflow when reconstructing the
64-bit offset from the two 32-bit values syscall arguments, and I believe
the patch below would fix this issue. Unfortunately I don't have in the
way of experience contributing to the kernel, so I would appreciate it if
someone would point out if I should

* not submit this at all,
* submit this somewhere else,
* fix it some other way,
* format this differently,
* test it using some method that I am unaware of,
* or am missing something else in general.

I've tried my best to align with the guidelines but if I've tripped up on
the details I would appreciate guidance on what I should be doing instead.

Thanks,
Saagar Jha

From c3525c7dfb9cede7cc246200ba70455855a3ec8b Mon Sep 17 00:00:00 2001
From: Saagar Jha <saagar@saagarjha.com>
Date: Sun, 12 Jan 2020 21:46:28 -0800
Subject: [PATCH] vfs: prevent signed overflow by using u64 over loff_t

32-bit system calls taking a 64-bit offset that arrive as split over two
32-bit unsigned integers overflow the signed loff_t when shifted over by
32 bits. Using unsigned intermediate types fixes the undefined behavior.

Signed-off-by: Saagar Jha <saagar@saagarjha.com>
---
 fs/read_write.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 5bbf587f5bc1..3a1dfafcaf65 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -347,7 +347,7 @@ SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high,
 	if (whence > SEEK_MAX)
 		goto out_putf;

-	offset = vfs_llseek(f.file, ((loff_t) offset_high << 32) | offset_low,
+	offset = vfs_llseek(f.file, ((u64) offset_high << 32) | offset_low,
 			whence);

 	retval = (int)offset;
@@ -1250,7 +1250,7 @@ COMPAT_SYSCALL_DEFINE5(preadv, compat_ulong_t, fd,
 		const struct compat_iovec __user *,vec,
 		compat_ulong_t, vlen, u32, pos_low, u32, pos_high)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = (((u64)pos_high << 32) | pos_low;

 	return do_compat_preadv64(fd, vec, vlen, pos, 0);
 }
@@ -1272,7 +1272,7 @@ COMPAT_SYSCALL_DEFINE6(preadv2, compat_ulong_t, fd,
 		compat_ulong_t, vlen, u32, pos_low, u32, pos_high,
 		rwf_t, flags)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = ((u64)pos_high << 32) | pos_low;

 	if (pos == -1)
 		return do_compat_readv(fd, vec, vlen, flags);
@@ -1359,7 +1359,7 @@ COMPAT_SYSCALL_DEFINE5(pwritev, compat_ulong_t, fd,
 		const struct compat_iovec __user *,vec,
 		compat_ulong_t, vlen, u32, pos_low, u32, pos_high)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = ((u64)pos_high << 32) | pos_low;

 	return do_compat_pwritev64(fd, vec, vlen, pos, 0);
 }
@@ -1380,7 +1380,7 @@ COMPAT_SYSCALL_DEFINE6(pwritev2, compat_ulong_t, fd,
 		const struct compat_iovec __user *,vec,
 		compat_ulong_t, vlen, u32, pos_low, u32, pos_high, rwf_t, flags)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = ((u64)pos_high << 32) | pos_low;

 	if (pos == -1)
 		return do_compat_writev(fd, vec, vlen, flags);
--
2.24.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread