linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] fs: fix unintentional arithmetic wraparound in offset calculation
@ 2024-05-09 23:42 Justin Stitt
  2024-05-13 20:01 ` Kees Cook
  0 siblings, 1 reply; 4+ messages in thread
From: Justin Stitt @ 2024-05-09 23:42 UTC (permalink / raw)
  To: Alexander Viro, Christian Brauner, Jan Kara, Nathan Chancellor,
	Bill Wendling
  Cc: linux-fsdevel, linux-kernel, llvm, linux-hardening, Justin Stitt

When running syzkaller with the newly reintroduced signed integer
overflow sanitizer we encounter this report:

[   67.995501] UBSAN: signed-integer-overflow in ../fs/read_write.c:91:10
[   68.000067] 9223372036854775807 + 4096 cannot be represented in type 'loff_t' (aka 'long long')
[   68.006266] CPU: 4 PID: 10851 Comm: syz-executor.5 Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1
[   68.012353] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   68.018983] Call Trace:
[   68.020803]  <TASK>
[   68.022540]  dump_stack_lvl+0x93/0xd0
[   68.025222]  handle_overflow+0x171/0x1b0
[   68.028053]  generic_file_llseek_size+0x35b/0x380

amongst others:
UBSAN: signed-integer-overflow in ../fs/read_write.c:1657:12
142606336 - -9223372036854775807 cannot be represented in type 'loff_t' (aka 'long long')
...
UBSAN: signed-integer-overflow in ../fs/read_write.c:1666:11
9223372036854775807 - -9223231299366420479 cannot be represented in type 'loff_t' (aka 'long long')

Historically, the signed integer overflow sanitizer did not work in the
kernel due to its interaction with `-fwrapv` but this has since been
changed [1] in the newest version of Clang. It was re-enabled in the
kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow
sanitizer").

Fix the accidental overflow in these position and offset calculations
by checking for negative position values, using check_add_overflow()
helpers and clamping values to expected ranges.

Since @offset is later limited by @maxsize, we can proactively safeguard
against exceeding that value (and by extension avoiding integer overflow):
	loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
	{
		if (offset < 0 && !unsigned_offsets(file))
			return -EINVAL;
		if (offset > maxsize)
			return -EINVAL;
		...

Link: https://github.com/llvm/llvm-project/pull/82432 [1]
Closes: https://github.com/KSPP/linux/issues/358
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Changes in v2:
- fix some more cases syzkaller found in read_write.c
- use min over min_t as the types are the same
- Link to v1: https://lore.kernel.org/r/20240509-b4-sio-read_write-v1-1-06bec2022697@google.com
---
Here's the syzkaller reproducer:
| # {Threaded:false Repeat:false RepeatTimes:0 Procs:1 Slowdown:1 Sandbox:
| # SandboxArg:0 Leak:false NetInjection:false NetDevices:false
| # NetReset:false Cgroups:false BinfmtMisc:false CloseFDs:false KCSAN:false
| # DevlinkPCI:false NicVF:false USB:false VhciInjection:false Wifi:false
| # IEEE802154:false Sysctl:false Swap:false UseTmpDir:false
| # HandleSegv:false Repro:false Trace:false LegacyOptions:{Collide:false
| # Fault:false FaultCall:0 FaultNth:0}}
| r0 = openat$sysfs(0xffffffffffffff9c, &(0x7f0000000000)='/sys/kernel/address_bits', 0x0, 0x98)
| lseek(r0, 0x7fffffffffffffff, 0x2)

... which was used against Kees' tree here (v6.8rc2):
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=wip/v6.9-rc2/unsigned-overflow-sanitizer

... with this config:
https://gist.github.com/JustinStitt/824976568b0f228ccbcbe49f3dee9bf4
---
 fs/read_write.c  | 18 +++++++++++-------
 fs/remap_range.c | 12 ++++++------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index d4c036e82b6c..d116e6e3eb3d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -88,7 +88,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 {
 	switch (whence) {
 	case SEEK_END:
-		offset += eof;
+		offset = min(offset, maxsize - eof) + eof;
 		break;
 	case SEEK_CUR:
 		/*
@@ -105,7 +105,8 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 		 * like SEEK_SET.
 		 */
 		spin_lock(&file->f_lock);
-		offset = vfs_setpos(file, file->f_pos + offset, maxsize);
+		offset = vfs_setpos(file, min(file->f_pos, maxsize - offset) +
+					      offset, maxsize);
 		spin_unlock(&file->f_lock);
 		return offset;
 	case SEEK_DATA:
@@ -1416,7 +1417,7 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
 	uint64_t count = *req_count;
-	loff_t size_in;
+	loff_t size_in, in_sum, out_sum;
 	int ret;
 
 	ret = generic_file_rw_checks(file_in, file_out);
@@ -1450,8 +1451,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
 	if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
 		return -ETXTBSY;
 
-	/* Ensure offsets don't wrap. */
-	if (pos_in + count < pos_in || pos_out + count < pos_out)
+	if (check_add_overflow(pos_in, count, &in_sum) ||
+	    check_add_overflow(pos_out, count, &out_sum))
 		return -EOVERFLOW;
 
 	/* Shorten the copy to EOF */
@@ -1467,8 +1468,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
 
 	/* Don't allow overlapped copying within the same file. */
 	if (inode_in == inode_out &&
-	    pos_out + count > pos_in &&
-	    pos_out < pos_in + count)
+	    out_sum > pos_in &&
+	    pos_out < in_sum)
 		return -EINVAL;
 
 	*req_count = count;
@@ -1649,6 +1650,9 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)
 	loff_t max_size = inode->i_sb->s_maxbytes;
 	loff_t limit = rlimit(RLIMIT_FSIZE);
 
+	if (pos < 0)
+		return -EINVAL;
+
 	if (limit != RLIM_INFINITY) {
 		if (pos >= limit) {
 			send_sig(SIGXFSZ, current, 0);
diff --git a/fs/remap_range.c b/fs/remap_range.c
index de07f978ce3e..4570be4ef463 100644
--- a/fs/remap_range.c
+++ b/fs/remap_range.c
@@ -36,7 +36,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
 	struct inode *inode_out = file_out->f_mapping->host;
 	uint64_t count = *req_count;
 	uint64_t bcount;
-	loff_t size_in, size_out;
+	loff_t size_in, size_out, in_sum, out_sum;
 	loff_t bs = inode_out->i_sb->s_blocksize;
 	int ret;
 
@@ -44,17 +44,17 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
 	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs))
 		return -EINVAL;
 
-	/* Ensure offsets don't wrap. */
-	if (pos_in + count < pos_in || pos_out + count < pos_out)
-		return -EINVAL;
+	if (check_add_overflow(pos_in, count, &in_sum) ||
+	    check_add_overflow(pos_out, count, &out_sum))
+		return -EOVERFLOW;
 
 	size_in = i_size_read(inode_in);
 	size_out = i_size_read(inode_out);
 
 	/* Dedupe requires both ranges to be within EOF. */
 	if ((remap_flags & REMAP_FILE_DEDUP) &&
-	    (pos_in >= size_in || pos_in + count > size_in ||
-	     pos_out >= size_out || pos_out + count > size_out))
+	    (pos_in >= size_in || in_sum > size_in ||
+	     pos_out >= size_out || out_sum > size_out))
 		return -EINVAL;
 
 	/* Ensure the infile range is within the infile. */

---
base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc
change-id: 20240509-b4-sio-read_write-04a17d40620e

Best regards,
--
Justin Stitt <justinstitt@google.com>


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

* Re: [PATCH v2] fs: fix unintentional arithmetic wraparound in offset calculation
  2024-05-09 23:42 [PATCH v2] fs: fix unintentional arithmetic wraparound in offset calculation Justin Stitt
@ 2024-05-13 20:01 ` Kees Cook
  2024-05-13 22:06   ` Justin Stitt
  0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2024-05-13 20:01 UTC (permalink / raw)
  To: Justin Stitt
  Cc: Alexander Viro, Christian Brauner, Jan Kara, Nathan Chancellor,
	Bill Wendling, linux-fsdevel, linux-kernel, llvm,
	linux-hardening

On Thu, May 09, 2024 at 11:42:07PM +0000, Justin Stitt wrote:
> When running syzkaller with the newly reintroduced signed integer
> overflow sanitizer we encounter this report:
> 
> [   67.995501] UBSAN: signed-integer-overflow in ../fs/read_write.c:91:10
> [   68.000067] 9223372036854775807 + 4096 cannot be represented in type 'loff_t' (aka 'long long')
> [   68.006266] CPU: 4 PID: 10851 Comm: syz-executor.5 Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1
> [   68.012353] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> [   68.018983] Call Trace:
> [   68.020803]  <TASK>
> [   68.022540]  dump_stack_lvl+0x93/0xd0
> [   68.025222]  handle_overflow+0x171/0x1b0
> [   68.028053]  generic_file_llseek_size+0x35b/0x380
> 
> amongst others:
> UBSAN: signed-integer-overflow in ../fs/read_write.c:1657:12
> 142606336 - -9223372036854775807 cannot be represented in type 'loff_t' (aka 'long long')
> ...
> UBSAN: signed-integer-overflow in ../fs/read_write.c:1666:11
> 9223372036854775807 - -9223231299366420479 cannot be represented in type 'loff_t' (aka 'long long')
> 
> Historically, the signed integer overflow sanitizer did not work in the
> kernel due to its interaction with `-fwrapv` but this has since been
> changed [1] in the newest version of Clang. It was re-enabled in the
> kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow
> sanitizer").
> 
> Fix the accidental overflow in these position and offset calculations
> by checking for negative position values, using check_add_overflow()
> helpers and clamping values to expected ranges.
> 
> Since @offset is later limited by @maxsize, we can proactively safeguard
> against exceeding that value (and by extension avoiding integer overflow):
> 	loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize)
> 	{
> 		if (offset < 0 && !unsigned_offsets(file))
> 			return -EINVAL;
> 		if (offset > maxsize)
> 			return -EINVAL;
> 		...
> 
> Link: https://github.com/llvm/llvm-project/pull/82432 [1]
> Closes: https://github.com/KSPP/linux/issues/358
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Changes in v2:
> - fix some more cases syzkaller found in read_write.c
> - use min over min_t as the types are the same
> - Link to v1: https://lore.kernel.org/r/20240509-b4-sio-read_write-v1-1-06bec2022697@google.com
> ---
> Here's the syzkaller reproducer:
> | # {Threaded:false Repeat:false RepeatTimes:0 Procs:1 Slowdown:1 Sandbox:
> | # SandboxArg:0 Leak:false NetInjection:false NetDevices:false
> | # NetReset:false Cgroups:false BinfmtMisc:false CloseFDs:false KCSAN:false
> | # DevlinkPCI:false NicVF:false USB:false VhciInjection:false Wifi:false
> | # IEEE802154:false Sysctl:false Swap:false UseTmpDir:false
> | # HandleSegv:false Repro:false Trace:false LegacyOptions:{Collide:false
> | # Fault:false FaultCall:0 FaultNth:0}}
> | r0 = openat$sysfs(0xffffffffffffff9c, &(0x7f0000000000)='/sys/kernel/address_bits', 0x0, 0x98)
> | lseek(r0, 0x7fffffffffffffff, 0x2)
> 
> ... which was used against Kees' tree here (v6.8rc2):
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=wip/v6.9-rc2/unsigned-overflow-sanitizer
> 
> ... with this config:
> https://gist.github.com/JustinStitt/824976568b0f228ccbcbe49f3dee9bf4
> ---
>  fs/read_write.c  | 18 +++++++++++-------
>  fs/remap_range.c | 12 ++++++------
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index d4c036e82b6c..d116e6e3eb3d 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -88,7 +88,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
>  {
>  	switch (whence) {
>  	case SEEK_END:
> -		offset += eof;
> +		offset = min(offset, maxsize - eof) + eof;

This seems effectively unchanged compared to v1?

https://lore.kernel.org/all/CAFhGd8qbUYXmgiFuLGQ7dWXFUtZacvT82wD4jSS-xNTvtzXKGQ@mail.gmail.com/

>  		break;
>  	case SEEK_CUR:
>  		/*
> @@ -105,7 +105,8 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
>  		 * like SEEK_SET.
>  		 */
>  		spin_lock(&file->f_lock);
> -		offset = vfs_setpos(file, file->f_pos + offset, maxsize);
> +		offset = vfs_setpos(file, min(file->f_pos, maxsize - offset) +
> +					      offset, maxsize);
>  		spin_unlock(&file->f_lock);
>  		return offset;
>  	case SEEK_DATA:
> @@ -1416,7 +1417,7 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>  	struct inode *inode_in = file_inode(file_in);
>  	struct inode *inode_out = file_inode(file_out);
>  	uint64_t count = *req_count;
> -	loff_t size_in;
> +	loff_t size_in, in_sum, out_sum;
>  	int ret;
>  
>  	ret = generic_file_rw_checks(file_in, file_out);
> @@ -1450,8 +1451,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>  	if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
>  		return -ETXTBSY;
>  
> -	/* Ensure offsets don't wrap. */
> -	if (pos_in + count < pos_in || pos_out + count < pos_out)
> +	if (check_add_overflow(pos_in, count, &in_sum) ||
> +	    check_add_overflow(pos_out, count, &out_sum))
>  		return -EOVERFLOW;

I like these changes -- they make this much more readable.

>  
>  	/* Shorten the copy to EOF */
> @@ -1467,8 +1468,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>  
>  	/* Don't allow overlapped copying within the same file. */
>  	if (inode_in == inode_out &&
> -	    pos_out + count > pos_in &&
> -	    pos_out < pos_in + count)
> +	    out_sum > pos_in &&
> +	    pos_out < in_sum)
>  		return -EINVAL;
>  
>  	*req_count = count;
> @@ -1649,6 +1650,9 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)
>  	loff_t max_size = inode->i_sb->s_maxbytes;
>  	loff_t limit = rlimit(RLIMIT_FSIZE);
>  
> +	if (pos < 0)
> +		return -EINVAL;
> +
>  	if (limit != RLIM_INFINITY) {
>  		if (pos >= limit) {
>  			send_sig(SIGXFSZ, current, 0);
> diff --git a/fs/remap_range.c b/fs/remap_range.c
> index de07f978ce3e..4570be4ef463 100644
> --- a/fs/remap_range.c
> +++ b/fs/remap_range.c
> @@ -36,7 +36,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
>  	struct inode *inode_out = file_out->f_mapping->host;
>  	uint64_t count = *req_count;
>  	uint64_t bcount;
> -	loff_t size_in, size_out;
> +	loff_t size_in, size_out, in_sum, out_sum;
>  	loff_t bs = inode_out->i_sb->s_blocksize;
>  	int ret;
>  
> @@ -44,17 +44,17 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
>  	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs))
>  		return -EINVAL;
>  
> -	/* Ensure offsets don't wrap. */
> -	if (pos_in + count < pos_in || pos_out + count < pos_out)
> -		return -EINVAL;
> +	if (check_add_overflow(pos_in, count, &in_sum) ||
> +	    check_add_overflow(pos_out, count, &out_sum))
> +		return -EOVERFLOW;

Yeah, this is a good error code change. This is ultimately exposed via
copy_file_range, where this error is documented as possible.

-Kees

-- 
Kees Cook

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

* Re: [PATCH v2] fs: fix unintentional arithmetic wraparound in offset calculation
  2024-05-13 20:01 ` Kees Cook
@ 2024-05-13 22:06   ` Justin Stitt
  2024-05-13 23:29     ` Kees Cook
  0 siblings, 1 reply; 4+ messages in thread
From: Justin Stitt @ 2024-05-13 22:06 UTC (permalink / raw)
  To: Kees Cook
  Cc: Alexander Viro, Christian Brauner, Jan Kara, Nathan Chancellor,
	Bill Wendling, linux-fsdevel, linux-kernel, llvm,
	linux-hardening

On Mon, May 13, 2024 at 01:01:57PM -0700, Kees Cook wrote:
> On Thu, May 09, 2024 at 11:42:07PM +0000, Justin Stitt wrote:
> >  fs/read_write.c  | 18 +++++++++++-------
> >  fs/remap_range.c | 12 ++++++------
> >  2 files changed, 17 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index d4c036e82b6c..d116e6e3eb3d 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -88,7 +88,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
> >  {
> >  	switch (whence) {
> >  	case SEEK_END:
> > -		offset += eof;
> > +		offset = min(offset, maxsize - eof) + eof;
> 
> This seems effectively unchanged compared to v1?
> 
> https://lore.kernel.org/all/CAFhGd8qbUYXmgiFuLGQ7dWXFUtZacvT82wD4jSS-xNTvtzXKGQ@mail.gmail.com/
> 

Right, please note the timestamps of Jan's review of v1 and when I sent
v2. Essentially, I sent v2 before Jan's review of v1 and as such v2 does
not fix the problem pointed out by Jan (the behavior of seek is
technically different for VERY LARGE offsets).

> >  		break;
> >  	case SEEK_CUR:
> >  		/*
> > @@ -105,7 +105,8 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
> >  		 * like SEEK_SET.
> >  		 */
> >  		spin_lock(&file->f_lock);
> > -		offset = vfs_setpos(file, file->f_pos + offset, maxsize);
> > +		offset = vfs_setpos(file, min(file->f_pos, maxsize - offset) +
> > +					      offset, maxsize);
> >  		spin_unlock(&file->f_lock);
> >  		return offset;
> >  	case SEEK_DATA:
> > @@ -1416,7 +1417,7 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
> >  	struct inode *inode_in = file_inode(file_in);
> >  	struct inode *inode_out = file_inode(file_out);
> >  	uint64_t count = *req_count;
> > -	loff_t size_in;
> > +	loff_t size_in, in_sum, out_sum;
> >  	int ret;
> >  
> >  	ret = generic_file_rw_checks(file_in, file_out);
> > @@ -1450,8 +1451,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
> >  	if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
> >  		return -ETXTBSY;
> >  
> > -	/* Ensure offsets don't wrap. */
> > -	if (pos_in + count < pos_in || pos_out + count < pos_out)
> > +	if (check_add_overflow(pos_in, count, &in_sum) ||
> > +	    check_add_overflow(pos_out, count, &out_sum))
> >  		return -EOVERFLOW;
> 
> I like these changes -- they make this much more readable.
> 
> >  
> >  	/* Shorten the copy to EOF */
> > @@ -1467,8 +1468,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
> >  
> >  	/* Don't allow overlapped copying within the same file. */
> >  	if (inode_in == inode_out &&
> > -	    pos_out + count > pos_in &&
> > -	    pos_out < pos_in + count)
> > +	    out_sum > pos_in &&
> > +	    pos_out < in_sum)
> >  		return -EINVAL;
> >  
> >  	*req_count = count;
> > @@ -1649,6 +1650,9 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)
> >  	loff_t max_size = inode->i_sb->s_maxbytes;
> >  	loff_t limit = rlimit(RLIMIT_FSIZE);
> >  
> > +	if (pos < 0)
> > +		return -EINVAL;
> > +
> >  	if (limit != RLIM_INFINITY) {
> >  		if (pos >= limit) {
> >  			send_sig(SIGXFSZ, current, 0);
> > diff --git a/fs/remap_range.c b/fs/remap_range.c
> > index de07f978ce3e..4570be4ef463 100644
> > --- a/fs/remap_range.c
> > +++ b/fs/remap_range.c
> > @@ -36,7 +36,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
> >  	struct inode *inode_out = file_out->f_mapping->host;
> >  	uint64_t count = *req_count;
> >  	uint64_t bcount;
> > -	loff_t size_in, size_out;
> > +	loff_t size_in, size_out, in_sum, out_sum;
> >  	loff_t bs = inode_out->i_sb->s_blocksize;
> >  	int ret;
> >  
> > @@ -44,17 +44,17 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
> >  	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs))
> >  		return -EINVAL;
> >  
> > -	/* Ensure offsets don't wrap. */
> > -	if (pos_in + count < pos_in || pos_out + count < pos_out)
> > -		return -EINVAL;
> > +	if (check_add_overflow(pos_in, count, &in_sum) ||
> > +	    check_add_overflow(pos_out, count, &out_sum))
> > +		return -EOVERFLOW;
> 
> Yeah, this is a good error code change. This is ultimately exposed via
> copy_file_range, where this error is documented as possible.
> 
> -Kees
> 
> -- 
> Kees Cook

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

* Re: [PATCH v2] fs: fix unintentional arithmetic wraparound in offset calculation
  2024-05-13 22:06   ` Justin Stitt
@ 2024-05-13 23:29     ` Kees Cook
  0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2024-05-13 23:29 UTC (permalink / raw)
  To: Justin Stitt
  Cc: Alexander Viro, Christian Brauner, Jan Kara, Nathan Chancellor,
	Bill Wendling, linux-fsdevel, linux-kernel, llvm,
	linux-hardening

On Mon, May 13, 2024 at 10:06:59PM +0000, Justin Stitt wrote:
> On Mon, May 13, 2024 at 01:01:57PM -0700, Kees Cook wrote:
> > On Thu, May 09, 2024 at 11:42:07PM +0000, Justin Stitt wrote:
> > >  fs/read_write.c  | 18 +++++++++++-------
> > >  fs/remap_range.c | 12 ++++++------
> > >  2 files changed, 17 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > index d4c036e82b6c..d116e6e3eb3d 100644
> > > --- a/fs/read_write.c
> > > +++ b/fs/read_write.c
> > > @@ -88,7 +88,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
> > >  {
> > >  	switch (whence) {
> > >  	case SEEK_END:
> > > -		offset += eof;
> > > +		offset = min(offset, maxsize - eof) + eof;
> > 
> > This seems effectively unchanged compared to v1?
> > 
> > https://lore.kernel.org/all/CAFhGd8qbUYXmgiFuLGQ7dWXFUtZacvT82wD4jSS-xNTvtzXKGQ@mail.gmail.com/
> > 
> 
> Right, please note the timestamps of Jan's review of v1 and when I sent
> v2. Essentially, I sent v2 before Jan's review of v1 and as such v2 does
> not fix the problem pointed out by Jan (the behavior of seek is
> technically different for VERY LARGE offsets).

Oh! Heh. I was tricked by versioning! ;)

-- 
Kees Cook

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

end of thread, other threads:[~2024-05-13 23:29 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-09 23:42 [PATCH v2] fs: fix unintentional arithmetic wraparound in offset calculation Justin Stitt
2024-05-13 20:01 ` Kees Cook
2024-05-13 22:06   ` Justin Stitt
2024-05-13 23:29     ` Kees Cook

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).