Skip to contentSkip to conversation

PATCH In review Andrew Morton 2 participants 4 messages Jun 28, 2018 – Jun 30, 2018 1 message not in the archive

Subsystems (2)
Memory management MM misc
lore
Share & tools
lore

Andrew Morton

author maintainer
#1 lore raw
In reply to a message not in the archive

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 32 lines · On Fri, 22 Jun 2018 23:37:27 +0000 [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=200209

Bug ID: 200209
Summary: UBSAN: Undefined behaviour in mm/fadvise.c:LINE
Product: Memory Management
Version: 2.5
Kernel Version: v4.18-rc2
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Page Allocator
Assignee: [email protected]
Reporter: [email protected]
Regression: No

Hi,
This bug was found in Linux Kernel v4.18-rc2

$ cat report0 
================================================================================
UBSAN: Undefined behaviour in mm/fadvise.c:76:10
signed integer overflow:
4 + 9223372036854775805 cannot be represented in type 'long long int'
CPU: 0 PID: 13477 Comm: syz-executor1 Not tainted 4.18.0-rc1 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x122/0x1c8 lib/dump_stack.c:113
Quoted 8 lines
 ubsan_epilogue+0x12/0x86 lib/ubsan.c:159
 handle_overflow+0x1c2/0x21f lib/ubsan.c:190
 __ubsan_handle_add_overflow+0x2a/0x31 lib/ubsan.c:198
 ksys_fadvise64_64+0xbf0/0xd10 mm/fadvise.c:76
 __do_sys_fadvise64 mm/fadvise.c:198 [inline]
 __se_sys_fadvise64 mm/fadvise.c:196 [inline]
 __x64_sys_fadvise64+0xa9/0x120 mm/fadvise.c:196
 do_syscall_64+0xb8/0x3a0 arch/x86/entry/common.c:290

That overflow is deliberate:

	endbyte = offset + len;
	if (!len || endbyte < len)
		endbyte = -1;
	else
		endbyte--;		/* inclusive */

Or is there a hole in this logic?

If not, I guess ee can do this another way to keep the checker happy.

#2 lore raw
Quoted 31 lines · Andrew Morton
Quoted 14 lines

Hi,
This bug was found in Linux Kernel v4.18-rc2

$ cat report0 
================================================================================
UBSAN: Undefined behaviour in mm/fadvise.c:76:10
signed integer overflow:
4 + 9223372036854775805 cannot be represented in type 'long long int'
CPU: 0 PID: 13477 Comm: syz-executor1 Not tainted 4.18.0-rc1 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x122/0x1c8 lib/dump_stack.c:113
Quoted 8 lines
 ubsan_epilogue+0x12/0x86 lib/ubsan.c:159
 handle_overflow+0x1c2/0x21f lib/ubsan.c:190
 __ubsan_handle_add_overflow+0x2a/0x31 lib/ubsan.c:198
 ksys_fadvise64_64+0xbf0/0xd10 mm/fadvise.c:76
 __do_sys_fadvise64 mm/fadvise.c:198 [inline]
 __se_sys_fadvise64 mm/fadvise.c:196 [inline]
 __x64_sys_fadvise64+0xa9/0x120 mm/fadvise.c:196
 do_syscall_64+0xb8/0x3a0 arch/x86/entry/common.c:290

That overflow is deliberate:

	endbyte = offset + len;
	if (!len || endbyte < len)
		endbyte = -1;
	else
		endbyte--;		/* inclusive */
Quoted 3 lines

Or is there a hole in this logic?

If not, I guess ee can do this another way to keep the checker happy.

It should be enough to make overflow unsigned. Unsigned overflow is defined by the C standard.

#3 lore raw

PATCH 1/1 mm/fadvise: Fix signed overflow UBSAN complaint

In reply to Andrew Morton

Signed integer overflow is undefined according to the C standard. The overflow in ksys_fadvise64_64() is deliberate, but since it is signed overflow, UBSAN complains:

	UBSAN: Undefined behaviour in mm/fadvise.c:76:10
	signed integer overflow:
	4 + 9223372036854775805 cannot be represented in type 'long long int'

Use unsigned types to do math. Unsigned overflow is defined so UBSAN will not complain about it. This patch doesn't change generated code.

Reported-by <[email protected]>
Signed-off-by Andrey Ryabinin <[email protected]>
---
 mm/fadvise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/fadvise.c +1 -1
index afa41491d324..1eaf2002d79a 100644
@@ -73,7 +73,7 @@ int ksys_fadvise64_64(int fd, loff_t offset, loff_t len, int advice)
 	}
 
 	/* Careful about overflows. Len == 0 means "as much as possible" */
-	endbyte = offset + len;
+	endbyte = (u64)offset + (u64)len;
 	if (!len || endbyte < len)
 		endbyte = -1;
 	else
Signature · 1 line
2.16.4
author maintainer
#4 lore raw

Re: [PATCH] mm/fadvise: Fix signed overflow UBSAN complaint

Quoted 15 lines · On Fri, 29 Jun 2018 21:44:53 +0300 Andrey Ryabinin <[email protected]>…

Signed integer overflow is undefined according to the C standard. The overflow in ksys_fadvise64_64() is deliberate, but since it is signed overflow, UBSAN complains:

	UBSAN: Undefined behaviour in mm/fadvise.c:76:10
	signed integer overflow:
	4 + 9223372036854775805 cannot be represented in type 'long long int'

Use unsigned types to do math. Unsigned overflow is defined so UBSAN will not complain about it. This patch doesn't change generated code.

...

mm/fadvise.c int ksys_fadvise64_64(int fd, loff_t offset, loff_t len, int advice) · line 79

Quoted 8 lines
mm/fadvise.c +1 -1
@@ -73,7 +73,7 @@ int ksys_fadvise64_64(int fd, loff_t offset, loff_t len, int advice)
 	}
 
 	/* Careful about overflows. Len == 0 means "as much as possible" */
-	endbyte = offset + len;
+	endbyte = (u64)offset + (u64)len;
 	if (!len || endbyte < len)
 		endbyte = -1;
 	else

Readers of this code will wonder "what the heck are those casts for". Therefore:

mm/fadvise.c renamed from mm/fadvise.c~mm-fadvise-fix-signed-overflow-ubsan-complaint-fix +5 -1
@@ -72,7 +72,11 @@ int ksys_fadvise64_64(int fd, loff_t off
 		goto out;
 	}
 
-	/* Careful about overflows. Len == 0 means "as much as possible" */
+	/*
+	 * Careful about overflows. Len == 0 means "as much as possible".  Use
+	 * unsigned math because signed overflows are undefined and UBSan
+	 * complains.
+	 */
 	endbyte = (u64)offset + (u64)len;
 	if (!len || endbyte < len)
 		endbyte = -1;

_