linux-kernel - Re: [PATCH v2] ubsan: don't handle misaligned address when support unaligned access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c7cc89e-f976-077b-7b64-1046a1f69569@virtuozzo.com>
Date:   Fri, 8 Dec 2017 13:48:53 +0300
From:   Andrey Ryabinin <aryabinin@...tuozzo.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Ding Tianhong <dingtianhong@...wei.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        LinuxArm <linuxarm@...wei.com>,
        David Laight <David.Laight@...LAB.COM>
Subject: Re: [PATCH v2] ubsan: don't handle misaligned address when support
 unaligned access



On 12/08/2017 02:24 AM, Andrew Morton wrote:
> On Thu, 7 Dec 2017 16:31:23 +0300 Andrey Ryabinin <aryabinin@...tuozzo.com> wrote:
> 
>> On 12/07/2017 03:49 AM, Andrew Morton wrote:
>>> (correcting Andrey's email address)
>>>
>>>
>>> From: Ding Tianhong <dingtianhong@...wei.com>
>>> Subject: lib/ubsan.c: don't handle misaligned address when kernel supports unaligned access
>>>
>>> ubsan reports a warning like:
>>>
>>> UBSAN: Undefined behaviour in ../include/linux/etherdevice.h:386:9
>>> load of misaligned address ffffffc069ba0482 for type 'long unsigned int'
>>> which requires 8 byte alignment
>>> CPU: 0 PID: 901 Comm: sshd Not tainted 4.xx+ #1
>>> Hardware name: linux,dummy-virt (DT)
>>> Call trace:
>>> [<ffffffc000093600>] dump_backtrace+0x0/0x348
>>> [<ffffffc000093968>] show_stack+0x20/0x30
>>> [<ffffffc001651664>] dump_stack+0x144/0x1b4
>>> [<ffffffc0016519b0>] ubsan_epilogue+0x18/0x74
>>> [<ffffffc001651bac>] __ubsan_handle_type_mismatch+0x1a0/0x25c
>>> [<ffffffc00125d8a0>] dev_gro_receive+0x17d8/0x1830
>>> [<ffffffc00125d928>] napi_gro_receive+0x30/0x158
>>> [<ffffffc000f4f93c>] virtnet_receive+0xad4/0x1fa8
>>>
>>> The reason is that when enabling the CONFIG_UBSAN_ALIGNMENT, ubsan will
>>> report the unaligned access even if the system supports it
>>> (CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y).  This produces a lot of noise
>>> in the log and causes confusion.
>>>
>>
>> NACK. This doesn't make sense. If you don't want to see misaligned access reports
>> you simply shouldn't enable CONFIG_UBSAN_ALIGNMENT.
> 
> So should UBSAN Kconfig disable CONFIG_UBSAN_ALIGNMENT when
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y?
> 

CONFIG_UBSAN_ALIGNMENT is already disabled by default for HAVE_EFFICIENT_UNALIGNED_ACCESS=y because it's noisy,
but we still allow users to enable it if they want to.

I don't think we should completely forbid enabling it for HAVE_EFFICIENT_UNALIGNED_ACCESS=y.
Unaligned access is still a bug in non-arch code and outside of sections like #ifdef HAVE_EFFICIENT_UNALIGNED_ACCESS .. #endif .

As for UBSAN noise inside #ifdef HAVE_EFFICIENT_UNALIGNED_ACCESS .. #endif sections, it should be possible to suppress it
with something like this:

	typedef __attribute__((aligned(1))) int unaligned_int;                                                                                                                                                              
	....

	int x = *(unaligned_int*)unalinged_addr;

This shouldn't affect generated code (on arches that support unaligned loads) and suppresses UBSAN warnings.
It's might be a right thing todo. Even if arch supports unaligned access, it's still undefined behaviour according to the C standard.
And one day, GCC might start doing optimizations based on this, e.g.:

	u64 *ptr;
	...
	x = *ptr;
	...
	if (ptr & 7)  // Compiler can assume that this statement is always false, because 'ptr' was deferenced, so it must be aligned
		do_something();