Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic

[Hao Ge <hao.ge@linux.dev>]

Hi  Suren


I think I understand what you mean now.

You're right. I think I shouldn't have included the unpoisoning process 
within the condition check for|phys_end < new_end|.

Give me a moment while I make the necessary modifications and verify the 
new version.


Thanks

Best Regards

Hao


On 12/5/24 23:34, Hao Ge wrote:
> Hi Suren
>
>
> On 12/5/24 22:48, Suren Baghdasaryan wrote:
>> On Wed, Dec 4, 2024 at 7:20 PM Hao Ge <hao.ge@linux.dev> wrote:
>>> Hi Suren
>>>
>>>
>>> On 12/5/24 10:14, Hao Ge wrote:
>>>> Hi Suren
>>>>
>>>>
>>>> On 12/5/24 03:33, Suren Baghdasaryan wrote:
>>>>> On Wed, Dec 4, 2024 at 7:08 AM Hao Ge <hao.ge@linux.dev> wrote:
>>>>>> Hi Suren
>>>>>>
>>>>>>
>>>>>> Thank you for your review.
>>>>>>
>>>>>>
>>>>>> On 12/4/24 22:39, Suren Baghdasaryan wrote:
>>>>>>> On Wed, Dec 4, 2024 at 12:35 AM Hao Ge <hao.ge@linux.dev> wrote:
>>>>>>>> From: Hao Ge <gehao@kylinos.cn>
>>>>>>>>
>>>>>>>> After merge commit 233e89322cbe ("alloc_tag:
>>>>>>>> fix module allocation tags populated area calculation"),
>>>>>>>> We still encountered a KASAN bug.
>>>>>>>>
>>>>>>>> This is because we have only actually performed
>>>>>>>> page allocation and address mapping here.
>>>>>>>> we need to unpoisoned portions of underlying memory.
>>>>>>>>
>>>>>>>> Because we have a change in the size here,we need to
>>>>>>>> re-annotate poisoned and unpoisoned portions of underlying memory
>>>>>>>> according to the new size.
>>>>>>>>
>>>>>>>> Here is the log for KASAN:
>>>>>>>>
>>>>>>>> [    5.041171][    T1]
>>>>>>>> ==================================================================
>>>>>>>> [    5.042047][    T1] BUG: KASAN: vmalloc-out-of-bounds in
>>>>>>>> move_module+0x2c0/0x708
>>>>>>>> [    5.042723][    T1] Write of size 240 at addr ffff80007e510000
>>>>>>>> by task systemd/1
>>>>>>>> [    5.043412][    T1]
>>>>>>>> [    5.043523][   T72] input: QEMU QEMU USB Tablet as
>>>>>>>> /devices/pci0000:00/0000:00:01.1/0000:02:001
>>>>>>>> [    5.043614][    T1] CPU: 0 UID: 0 PID: 1 Comm: systemd Not
>>>>>>>> tainted 6.13.0-rc1+ #28
>>>>>>>> [    5.045560][    T1] Hardware name: QEMU KVM Virtual Machine,
>>>>>>>> BIOS 0.0.0 02/06/2015
>>>>>>>> [    5.046328][    T1] Call trace:
>>>>>>>> [    5.046670][    T1]  show_stack+0x20/0x38 (C)
>>>>>>>> [    5.047127][    T1]  dump_stack_lvl+0x80/0xf8
>>>>>>>> [    5.047533][    T1]
>>>>>>>> print_address_description.constprop.0+0x58/0x358
>>>>>>>> [    5.048092][   T72] hid-generic 0003:0627:0001.0001:
>>>>>>>> input,hidraw0: USB HID v0.01 Mouse [QEMU 0
>>>>>>>> [    5.048126][    T1]  print_report+0xb0/0x280
>>>>>>>> [    5.049682][    T1]  kasan_report+0xb8/0x108
>>>>>>>> [    5.050170][    T1]  kasan_check_range+0xe8/0x190
>>>>>>>> [    5.050685][    T1]  memcpy+0x58/0xa0
>>>>>>>> [    5.051135][    T1]  move_module+0x2c0/0x708
>>>>>>>> [    5.051586][    T1] layout_and_allocate.constprop.0+0x308/0x5b8
>>>>>>>> [    5.052219][    T1]  load_module+0x134/0x16c8
>>>>>>>> [    5.052671][    T1] init_module_from_file+0xdc/0x138
>>>>>>>> [    5.053193][    T1] idempotent_init_module+0x344/0x600
>>>>>>>> [    5.053742][    T1] __arm64_sys_finit_module+0xbc/0x150
>>>>>>>> [    5.054289][    T1]  invoke_syscall+0xd4/0x258
>>>>>>>> [    5.054749][    T1] el0_svc_common.constprop.0+0xb4/0x240
>>>>>>>> [    5.055319][    T1]  do_el0_svc+0x48/0x68
>>>>>>>> [    5.055743][    T1]  el0_svc+0x40/0xe0
>>>>>>>> [    5.056142][    T1] el0t_64_sync_handler+0x10c/0x138
>>>>>>>> [    5.056658][    T1]  el0t_64_sync+0x1ac/0x1b0
>>>>>>>>
>>>>>>>> Fixes: 233e89322cbe ("alloc_tag: fix module allocation tags
>>>>>>>> populated area calculation")
>>>>>>>> Signed-off-by: Hao Ge <gehao@kylinos.cn>
>>>>>>> Thanks for the fix!
>>>>>>>
>>>>>>>> ---
>>>>>>>> v2: Add comments to kasan_unpoison_vmalloc like other places.
>>>>>>>>
>>>>>>>> commit 233e89322cbe ("alloc_tag: fix module allocation
>>>>>>>> tags populated area calculation") is currently in the
>>>>>>>> mm-hotfixes-unstable branch, so this patch is
>>>>>>>> developed based on the mm-hotfixes-unstable branch.
>>>>>>>> ---
>>>>>>>>     lib/alloc_tag.c | 13 +++++++++++++
>>>>>>>>     1 file changed, 13 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
>>>>>>>> index 4ee6caa6d2da..f885b3f3af0e 100644
>>>>>>>> --- a/lib/alloc_tag.c
>>>>>>>> +++ b/lib/alloc_tag.c
>>>>>>>> @@ -421,7 +421,20 @@ static int vm_module_tags_populate(void)
>>>>>>>> __free_page(next_page[i]);
>>>>>>>>                            return -ENOMEM;
>>>>>>>>                    }
>>>>>>>> +
>>>>>>>> +               kasan_poison_vmalloc((void 
>>>>>>>> *)module_tags.start_addr,
>>>>>>>> + vm_module_tags->nr_pages << PAGE_SHIFT);
>>>>>>>> +
>>>>>>>>                    vm_module_tags->nr_pages += nr;
>>>>>>>> +
>>>>>>>> +               /*
>>>>>>>> +                * Mark the pages as accessible, now that they are
>>>>>>>> mapped.
>>>>>>>> +                * With hardware tag-based KASAN, marking is
>>>>>>>> skipped for
>>>>>>>> +                * non-VM_ALLOC mappings, see
>>>>>>>> __kasan_unpoison_vmalloc().
>>>>>>>> +                */
>>>>>>>> +               kasan_unpoison_vmalloc((void
>>>>>>>> *)module_tags.start_addr,
>>>>>>>> + vm_module_tags->nr_pages << PAGE_SHIFT,
>>>>>>>> + KASAN_VMALLOC_PROT_NORMAL);
>>>>>>> Instead of poisoning [module_tags.start_addr,
>>>>>>> vm_module_tags->nr_pages], incrementing vm_module_tags->nr_pages 
>>>>>>> and
>>>>>>> the unpoisoning [module_tags.start_addr, vm_module_tags->nr_pages]
>>>>>>> could we simply poisons the additional area like this:
>>>>>>>
>>>>>>>                    kasan_unpoison_vmalloc((void
>>>>>>> *)module_tags.start_addr +
>>>>>>> (vm_module_tags->nr_pages << PAGE_SHIFT),
>>>>>>>                                           nr << PAGE_SHIFT,
>>>>>>> KASAN_VMALLOC_PROT_NORMAL);
>>>>>>>                   vm_module_tags->nr_pages += nr;
>>>>>>> ?
>>>>>> I had considered making such modifications earlier.
>>>>>>
>>>>>> But considering the following situation,
>>>>>>
>>>>>> A module tags spans across the regions of [module_tags.start_addr,
>>>>>> vm_module_tags->nr_pages] and [module_tags.start_addr +
>>>>>> vm_module_tags->nr_pages, ...].
>>>>>>
>>>>>> It may result in false positives for out-of-bounds errors.
>>>>> Sorry, maybe I'm missing something but I don't see why poisoning only
>>>>> newly mapped area would lead to false positives. Could you please
>>>>> clarify?
>>>>
>>>> Because KASAN may perceive the two as distinct address spaces, despite
>>>> their addresses being contiguous.
>>>>
>>>> So, when a module tag spans across these two contiguous address
>>>> spaces, KASAN may incorrectly consider it as an out-of-bounds access.
>>>>
>>>>
>>>>> Also, if you do need to unpoison and then poison, using phys_end and
>>>>> new_end would be better, like this:
>>>>>
>>>>> kasan_poison_vmalloc((void *)module_tags.start_addr,
>>>>>                                         phys_end -
>>>>> module_tags.start_addr)
>>>>>
>>>>> kasan_unpoison_vmalloc((void *)module_tags.start_addr,
>>>>>                                             new_end -
>>>>> module_tags.start_addr,
>>>>> KASAN_VMALLOC_PROT_NORMAL);
>>>> OK, the next version will include.
>>> After verification and consideration, I have found that this
>>> modification may still pose problems.
>>>
>>> Because we haven't ensured that  new_end is page-aligned,
>>>
>>> So, we've only made the region from||module_tags.start_addr
>>> tonew_endaccessible.
>> Correct and the area [module_tags.start_addr, new_end] is the one that
>> should be considered valid/accessible. We fault-in a physical page
>> that includes new_end and might cover some area after that address but
>> accessing the addresses above new_end is technically out-of-bounds
>> (there are no valid codetags there).
>>
>>> Using this example, in reality,end equals 0xffff80007e5100f0:
>>>
>>> Write of size 240 at addr ffff80007e510000 by task systemd/1
>>>
>>> When we access other memory within the same page as0xffff80007e5100f0,
>>> KASAN warnings will also be issued due to the lack of unpoisoned
>>> portions in that memory.
>> Will you get a KASAN warning if you access memory below new_end?
>> Warnings above that address I think should be considered as expected
>> (even though we have a valid physical page there).
>> Does that make sense?
>
> Is that really the case?
>
> Here is the log that has been updated to include the calculation 
> new_end - module_tags.start_addr.
>
> On my machine,module_tags.start_addr is equal to ffff80007e510000
>
> and the size of the first module_tags is 240
>
> So, because you only made the range|[module_tags.start_addr, 
> new_end]|accessible, the same issue will arise again later on.
>
> [    5.798918][  T258] BUG: KASAN: vmalloc-out-of-bounds in 
> move_module+0x2c0/0x708
> [    5.799622][  T258] Write of size 200 at addr ffff80007e5100f0 by 
> task systemd-modules/258
> [    5.800149][  T256] systemd-journald[256]: Collecting audit 
> messages is disabled.
> [    5.800296][  T258]
> [    5.800301][  T258] CPU: 2 UID: 0 PID: 258 Comm: systemd-modules 
> Not tainted 6.13.0-rc1+ #46
> [    5.801727][   T10] input: QEMU QEMU USB Mouse as 
> /devices/pci0000:00/0000:00:01.1/0000:02:00.3
> [    5.801905][  T258] Hardware name: QEMU KVM Virtual Machine, BIOS 
> 0.0.0 02/06/2015
> [    5.801911][  T258] Call trace:
> [    5.804120][  T258]  show_stack+0x20/0x38 (C)
> [    5.804512][  T258]  dump_stack_lvl+0x80/0xf8
> [    5.804916][  T258] print_address_description.constprop.0+0x58/0x358
> [    5.805276][   T10] hid-generic 0003:0627:0001.0003: input,hidraw2: 
> USB HID v0.01 Mouse [QEMU 0
> [    5.805501][  T258]  print_report+0xb0/0x280
> [    5.807031][  T258]  kasan_report+0xb8/0x108
> [    5.807415][  T258]  kasan_check_range+0xe8/0x190
> [    5.807714][  T124] pcieport 0000:00:02.3: pciehp: Slot(0-11): No 
> device found
> [    5.807921][  T258]  memcpy+0x58/0xa0
> [    5.807927][  T258]  move_module+0x2c0/0x708
> [    5.809346][  T258] layout_and_allocate.constprop.0+0x308/0x5b8
> [    5.809942][  T258]  load_module+0x134/0x16c8
> [    5.810375][  T258]  init_module_from_file+0xdc/0x138
> [    5.810870][  T258]  idempotent_init_module+0x344/0x600
> [    5.811389][  T258]  __arm64_sys_finit_module+0xbc/0x150
> [    5.811916][  T258]  invoke_syscall+0xd4/0x258
> [    5.812362][  T258]  el0_svc_common.constprop.0+0xb4/0x240
> [    5.812914][  T258]  do_el0_svc+0x48/0x68
> [    5.813318][  T258]  el0_svc+0x40/0xe0
> [    5.813698][  T258]  el0t_64_sync_handler+0x10c/0x138
> [    5.814210][  T258]  el0t_64_sync+0x1ac/0x1b0[    5.798918][ T258] 
> BUG: KASAN: vmalloc-out-of-bounds in move_module+0x2c0/0x708
> [    5.799622][  T258] Write of size 200 at addr ffff80007e5100f0 by 
> task systemd-modules/258
> [    5.800149][  T256] systemd-journald[256]: Collecting audit 
> messages is disabled.
> [    5.800296][  T258]
> [    5.800301][  T258] CPU: 2 UID: 0 PID: 258 Comm: systemd-modules 
> Not tainted 6.13.0-rc1+ #46
> [    5.801727][   T10] input: QEMU QEMU USB Mouse as 
> /devices/pci0000:00/0000:00:01.1/0000:02:00.3
> [    5.801905][  T258] Hardware name: QEMU KVM Virtual Machine, BIOS 
> 0.0.0 02/06/2015
> [    5.801911][  T258] Call trace:
> [    5.804120][  T258]  show_stack+0x20/0x38 (C)
> [    5.804512][  T258]  dump_stack_lvl+0x80/0xf8
> [    5.804916][  T258] print_address_description.constprop.0+0x58/0x358
> [    5.805276][   T10] hid-generic 0003:0627:0001.0003: input,hidraw2: 
> USB HID v0.01 Mouse [QEMU 0
> [    5.805501][  T258]  print_report+0xb0/0x280
> [    5.807031][  T258]  kasan_report+0xb8/0x108
> [    5.807415][  T258]  kasan_check_range+0xe8/0x190
> [    5.807714][  T124] pcieport 0000:00:02.3: pciehp: Slot(0-11): No 
> device found
> [    5.807921][  T258]  memcpy+0x58/0xa0
> [    5.807927][  T258]  move_module+0x2c0/0x708
> [    5.809346][  T258] layout_and_allocate.constprop.0+0x308/0x5b8
> [    5.809942][  T258]  load_module+0x134/0x16c8
> [    5.810375][  T258]  init_module_from_file+0xdc/0x138
> [    5.810870][  T258]  idempotent_init_module+0x344/0x600
> [    5.811389][  T258]  __arm64_sys_finit_module+0xbc/0x150
> [    5.811916][  T258]  invoke_syscall+0xd4/0x258
> [    5.812362][  T258]  el0_svc_common.constprop.0+0xb4/0x240
> [    5.812914][  T258]  do_el0_svc+0x48/0x68
> [    5.813318][  T258]  el0_svc+0x40/0xe0
> [    5.813698][  T258]  el0t_64_sync_handler+0x10c/0x138
> [    5.814210][  T258]  el0t_64_sync+0x1ac/0x1b0
>
>>> Based on that, I would suggest sticking with the V2 version.
>>>
>>>
>>> Thanks
>>>
>>> Best Regards
>>>
>>> Hao
>>>
>>>>
>>>> Thanks
>>>>
>>>> Best regards
>>>>
>>>> Hao
>>>>
>>>>
>>>>>>>>            }
>>>>>>>>
>>>>>>>>            return 0;
>>>>>>>> -- 
>>>>>>>> 2.25.1
>>>>>>>>