| 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
| ||
|
Message-ID: <6dab626e-acee-9f4e-c97b-7a225882edff@linux.dev>
Date: Thu, 5 Dec 2024 10:14:54 +0800
From: Hao Ge <hao.ge@...ux.dev>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: kent.overstreet@...ux.dev, akpm@...ux-foundation.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Hao Ge <gehao@...inos.cn>
Subject: Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN
poisoning logic
Hi Suren
On 12/5/24 03:33, Suren Baghdasaryan wrote:
> On Wed, Dec 4, 2024 at 7:08 AM Hao Ge <hao.ge@...ux.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@...ux.dev> wrote:
>>>> From: Hao Ge <gehao@...inos.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@...inos.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.
Thanks
Best regards
Hao
>>
>>>> }
>>>>
>>>> return 0;
>>>> --
>>>> 2.25.1
>>>>
Powered by blists - more mailing lists