(no subject)

[Andrew Morton]

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

On Fri, 22 Jun 2018 23:37:27 +0000 bugzilla-daemon@bugzilla.kernel.org 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: akpm@linux-foundation.org
>           Reporter: icytxw@gmail.com
>         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
>  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.

Re:

[Andrey Ryabinin]

On 06/28/2018 06:48 AM, Andrew Morton wrote:

>> 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
>>  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.
 
It should be enough to make overflow unsigned. Unsigned overflow is defined by the C standard.

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

[Andrey Ryabinin]

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: <icytxw@gmail.com>
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 mm/fadvise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/fadvise.c b/mm/fadvise.c
index afa41491d324..1eaf2002d79a 100644
--- a/mm/fadvise.c
+++ b/mm/fadvise.c
@@ -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
-- 
2.16.4

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

[Andrew Morton]

On Fri, 29 Jun 2018 21:44:53 +0300 Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:

> 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.
> 
> ...
>
> --- a/mm/fadvise.c
> +++ b/mm/fadvise.c
> @@ -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:

--- a/mm/fadvise.c~mm-fadvise-fix-signed-overflow-ubsan-complaint-fix
+++ a/mm/fadvise.c
@@ -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;
_