linux-kernel - Re: [PATCH] asm/sections: fix the determination of the end of the memory region

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] [day] [month] [year] [list]

Date:   Tue, 16 Aug 2022 09:00:33 +0800
From:   Quanyang Wang <quanyang.wang@...driver.com>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thierry Reding <treding@...dia.com>,
        linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] asm/sections: fix the determination of the end of the
 memory region

Hi Ard,

On 8/11/22 16:16, Ard Biesheuvel wrote:
> On Thu, 11 Aug 2022 at 08:31, <quanyang.wang@...driver.com> wrote:
>>
>> From: Quanyang Wang <quanyang.wang@...driver.com>
>>
>> If using "vend >= begin" to judge if two memory regions intersects, vend
>> should be the end of the memory region, so it should be "virt + size -1"
>> instead of "virt + size".
>> The wrong determination of the end triggers the misreporting as below when
>> the dma debug function "check_for_illegal_area" calls memory_intersects to
>> check if the dma region intersects with stext region.
>>
>> Calltrace (stext is at 0x80100000):
>>   WARNING: CPU: 0 PID: 77 at kernel/dma/debug.c:1073 check_for_illegal_area+0x130/0x168
>>   DMA-API: chipidea-usb2 e0002000.usb: device driver maps memory from kernel text or rodata [addr=800f0000] [len=65536]
>>   Modules linked in:
>>   CPU: 1 PID: 77 Comm: usb-storage Not tainted 5.19.0-yocto-standard #5
>>   Hardware name: Xilinx Zynq Platform
>>    unwind_backtrace from show_stack+0x18/0x1c
>>    show_stack from dump_stack_lvl+0x58/0x70
>>    dump_stack_lvl from __warn+0xb0/0x198
>>    __warn from warn_slowpath_fmt+0x80/0xb4
>>    warn_slowpath_fmt from check_for_illegal_area+0x130/0x168
>>    check_for_illegal_area from debug_dma_map_sg+0x94/0x368
>>    debug_dma_map_sg from __dma_map_sg_attrs+0x114/0x128
>>    __dma_map_sg_attrs from dma_map_sg_attrs+0x18/0x24
>>    dma_map_sg_attrs from usb_hcd_map_urb_for_dma+0x250/0x3b4
>>    usb_hcd_map_urb_for_dma from usb_hcd_submit_urb+0x194/0x214
>>    usb_hcd_submit_urb from usb_sg_wait+0xa4/0x118
>>    usb_sg_wait from usb_stor_bulk_transfer_sglist+0xa0/0xec
>>    usb_stor_bulk_transfer_sglist from usb_stor_bulk_srb+0x38/0x70
>>    usb_stor_bulk_srb from usb_stor_Bulk_transport+0x150/0x360
>>    usb_stor_Bulk_transport from usb_stor_invoke_transport+0x38/0x440
>>    usb_stor_invoke_transport from usb_stor_control_thread+0x1e0/0x238
>>    usb_stor_control_thread from kthread+0xf8/0x104
>>    kthread from ret_from_fork+0x14/0x2c
>>
>> Fixes: 979559362516 ("asm/sections: add helpers to check for section data")
>> Signed-off-by: Quanyang Wang <quanyang.wang@...driver.com>
>> ---
>>   include/asm-generic/sections.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>> index d0f7bdd2fdf2..f7171b4f5bfd 100644
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -108,7 +108,7 @@ static inline bool memory_contains(void *begin, void *end, void *virt,
>>   static inline bool memory_intersects(void *begin, void *end, void *virt,
>>                                       size_t size)
>>   {
>> -       void *vend = virt + size;
>> +       void *vend = virt + size - 1;
>>
>>          return (virt >= begin && virt < end) || (vend >= begin && vend < end);
> 
> This test looks flawed to me for another reason as well: it only
> checks whether the start /or/ the end of (virt, virt+size) falls
> inside the area, so if the area is covered completely (in which case
> the intersection of the two will be equal to the area), this will
> return false erroneously.
Yes, the test lacks some checks. I will send a V2 patch to fix it.
Thanks,
Quanyang