diff mbox series

fs: fix unintentional arithmetic wraparound in offset calculation

Message ID 20240509-b4-sio-read_write-v1-1-06bec2022697@google.com (mailing list archive)
State Superseded
Headers show
Series fs: fix unintentional arithmetic wraparound in offset calculation | expand

Commit Message

Justin Stitt May 9, 2024, 9:34 p.m. UTC
When running syzkaller with the newly reintroduced signed integer
overflow sanitizer we encounter this report:

[   67.991989] ------------[ cut here ]------------
[   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
...

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

Since @offset is later limited by @maxsize, we can proactively safeguard
against exceeding that value and also dodge some accidental overflow
(which may cause bad file access):

	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>
---
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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


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

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

Comments

Jan Kara May 10, 2024, 3:15 p.m. UTC | #1
On Thu 09-05-24 21:34:58, Justin Stitt wrote:
> When running syzkaller with the newly reintroduced signed integer
> overflow sanitizer we encounter this report:
> 
> [   67.991989] ------------[ cut here ]------------
> [   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
> ...
> 
> 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").
> 
> Since @offset is later limited by @maxsize, we can proactively safeguard
> against exceeding that value and also dodge some accidental overflow
> (which may cause bad file access):
> 
> 	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>
> ---
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index d4c036e82b6c..10c3eaa5ef55 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_t(loff_t, offset, maxsize - eof) + eof;

Well, but by this you change the behavior of seek(2) for huge offsets.
Previously we'd return -EINVAL (from following vfs_setpos()), now we set
position to maxsize. I don't think that is desirable?

Also the addition in SEEK_CUR could overflow in the same way AFAICT so we
could treat that in one patch so that the whole function is fixed at once?

								Honza
Justin Stitt May 10, 2024, 4:21 p.m. UTC | #2
On Fri, May 10, 2024 at 8:15 AM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 09-05-24 21:34:58, Justin Stitt wrote:
> > ---
> >  fs/read_write.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index d4c036e82b6c..10c3eaa5ef55 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_t(loff_t, offset, maxsize - eof) + eof;
>
> Well, but by this you change the behavior of seek(2) for huge offsets.
> Previously we'd return -EINVAL (from following vfs_setpos()), now we set
> position to maxsize. I don't think that is desirable?

RIght, we shouldn't change the current behavior. This patch needs rethinking.

>
> Also the addition in SEEK_CUR could overflow in the same way AFAICT so we
> could treat that in one patch so that the whole function is fixed at once?

Yep let's include that one as well. However, I'm going to hold off on
sending a new version until the discussion about how to handle
overflow comes to a conclusion; as suggested by Greg [1]. I made too
many assumptions about how folks want overflow to be handled. In the
case of this patch, a simple check_add_overflow() should be okay and
match the behavior, but let's wait and see.

>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

[1]: https://lore.kernel.org/all/2024051039-bankable-liking-e836@gregkh/

Thanks
Justin
diff mbox series

Patch

diff --git a/fs/read_write.c b/fs/read_write.c
index d4c036e82b6c..10c3eaa5ef55 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_t(loff_t, offset, maxsize - eof) + eof;
 		break;
 	case SEEK_CUR:
 		/*