| Message ID | 20241204083448.387862-1-hao.ge@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic | expand |
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; ? > } > > return 0; > -- > 2.25.1 >
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. >> } >> >> return 0; >> -- >> 2.25.1 >>
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); } return 0;