UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56

[Paul Menzel]

Dear Christophe,


Am 07.05.21 um 10:59 schrieb Christophe Leroy:

> Le 07/05/2021 à 10:42, Paul Menzel a écrit :
>> [+Andrey]

>> Am 07.05.21 um 10:31 schrieb Christophe Leroy:
>>
>>> Le 06/05/2021 à 21:32, Paul Menzel a écrit :
>>>> [corrected subject]
>>>>
>>>> Am 06.05.21 um 21:31 schrieb Paul Menzel:
>>
>>>>> On the POWER8 system IBM S822LC, Linux 5.13+, built with USSAN, 
>>>>> logs the warning below.
>>>>>
>>>>> ```
>>>>> [    0.030091] ================================================================================
>>>>> [    0.030295] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
>>>>> [    0.030325] index -1 is out of range for type 'legacy_serial_info [8]'
>>>>> [    0.030350] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>>>>> [    0.030360] Call Trace:
>>>>> [    0.030363] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>>>>> [    0.030386] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>>>>> [    0.030400] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>>>>> [    0.030414] [c000000024f1bc20] [c000000001711588] ioremap_legacy_serial_console+0x54/0x144
>>>>> [    0.030430] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>>>>> [    0.030444] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>>>>> [    0.030458] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>>>>> [    0.030471] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>>>>> [    0.030484] ================================================================================
>>>>> [    0.030641] ================================================================================
>>>>> [    0.030668] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:360:58
>>>>> [    0.030697] index -1 is out of range for type 'plat_serial8250_port [9]'
>>>>> [    0.030721] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>>>>> [    0.030730] Call Trace:
>>>>> [    0.030733] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>>>>> [    0.030749] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>>>>> [    0.030762] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>>>>> [    0.030775] [c000000024f1bc20] [c0000000017115a0] ioremap_legacy_serial_console+0x6c/0x144
>>>>> [    0.030790] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>>>>> [    0.030802] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>>>>> [    0.030816] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>>>>> [    0.030829] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>>>>> [    0.030842] ================================================================================
>>>>> ```
>>>
>>> The function is as follows, so when legacy_serial_console == -1 as in 
>>> your situation, the pointers are just not used.
>>>
>>> static int __init ioremap_legacy_serial_console(void)
>>> {
>>>      struct legacy_serial_info *info = &legacy_serial_infos[legacy_serial_console];
>>>      struct plat_serial8250_port *port = &legacy_serial_ports[legacy_serial_console];
>>>      void __iomem *vaddr;
>>>
>>>      if (legacy_serial_console < 0)
>>>          return 0;
>>>
>>> ...
>>> }
>>>
>>> When I look into the generated code (UBSAN not selected), we see the 
>>> verification and the bail-out is done prior to any calculation based 
>>> on legacy_serial_console.
>>>
>>> 00000000 <ioremap_legacy_serial_console>:
>>>    0:    94 21 ff e0     stwu    r1,-32(r1)
>>>    4:    3d 20 00 00     lis     r9,0
>>>              6: R_PPC_ADDR16_HA    .data
>>>    8:    7c 08 02 a6     mflr    r0
>>>    c:    bf 81 00 10     stmw    r28,16(r1)
>>>   10:    3b 80 00 00     li      r28,0
>>>   14:    83 a9 00 00     lwz     r29,0(r9)
>>>              16: R_PPC_ADDR16_LO    .data
>>>   18:    90 01 00 24     stw     r0,36(r1)
>>>   1c:    2c 1d 00 00     cmpwi   r29,0
>>>   20:    41 80 00 80     blt     a0 <ioremap_legacy_serial_console+0xa0>
>>>
>>> So, is it normal that UBSAN reports an error here ?
>>
>> If it’s useful, I could disassemble the code here. But please tell me 
>> how.
>>
>> Sorry, I do not know. I just selected the option, and saw the error. 
>> Maybe Andrey has an idea.
> 
> No need for you to disassemble, I just wanted to show that without UBSAN 
> there is no problem with the index as it is used only after boundary 
> checking. (But if you want to do so, if is just an 'objdump -dr 
> legacy_serial.o')

Thank you for the hint.

> Now, with UBSAN, I see that UBSAN does the verification of the index 
> earlier than expected. So what to do here, we can modify the code, but 
> that modification would just be to make UBSAN happy as there is no 
> problem in itself.

In #gcc at irc.freenode.net I was told by zid (they weren’t so happy with 
the wording), but maybe you understand it:

> It's not legal C to generate pointers to things other than 0,
> objects, or 1 past the end of an object, not just dereference them,
> so technically that's not legal per the C spec.
> 
> In practice it won't matter until it's dereferenced of course unless
> you're doing something weird, let's say.. instrumenting the code

Kind regards,

Paul