* [PATCH] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
@ 2024-12-04 7:52 Hao Ge
2024-12-04 8:34 ` [PATCH v2] " Hao Ge
0 siblings, 1 reply; 12+ messages in thread
From: Hao Ge @ 2024-12-04 7:52 UTC (permalink / raw)
To: surenb, kent.overstreet, akpm; +Cc: linux-mm, linux-kernel, hao.ge, Hao Ge
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>
---
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 | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
index 4ee6caa6d2da..ad31acb1279a 100644
--- a/lib/alloc_tag.c
+++ b/lib/alloc_tag.c
@@ -421,7 +421,15 @@ 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;
+
+ kasan_unpoison_vmalloc((void *)module_tags.start_addr,
+ vm_module_tags->nr_pages << PAGE_SHIFT,
+ KASAN_VMALLOC_PROT_NORMAL);
}
return 0;
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-04 7:52 [PATCH] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic Hao Ge
@ 2024-12-04 8:34 ` Hao Ge
2024-12-04 14:39 ` Suren Baghdasaryan
0 siblings, 1 reply; 12+ messages in thread
From: Hao Ge @ 2024-12-04 8:34 UTC (permalink / raw)
To: surenb, kent.overstreet, akpm; +Cc: linux-mm, linux-kernel, hao.ge, Hao Ge
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>
---
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);
}
return 0;
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-04 8:34 ` [PATCH v2] " Hao Ge
@ 2024-12-04 14:39 ` Suren Baghdasaryan
2024-12-04 15:07 ` Hao Ge
0 siblings, 1 reply; 12+ messages in thread
From: Suren Baghdasaryan @ 2024-12-04 14:39 UTC (permalink / raw)
To: Hao Ge; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-04 14:39 ` Suren Baghdasaryan
@ 2024-12-04 15:07 ` Hao Ge
2024-12-04 19:33 ` Suren Baghdasaryan
0 siblings, 1 reply; 12+ messages in thread
From: Hao Ge @ 2024-12-04 15:07 UTC (permalink / raw)
To: Suren Baghdasaryan; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
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
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-04 15:07 ` Hao Ge
@ 2024-12-04 19:33 ` Suren Baghdasaryan
2024-12-05 2:14 ` Hao Ge
0 siblings, 1 reply; 12+ messages in thread
From: Suren Baghdasaryan @ 2024-12-04 19:33 UTC (permalink / raw)
To: Hao Ge; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
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?
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);
>
>
> >> }
> >>
> >> return 0;
> >> --
> >> 2.25.1
> >>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-04 19:33 ` Suren Baghdasaryan
@ 2024-12-05 2:14 ` Hao Ge
2024-12-05 3:20 ` Hao Ge
0 siblings, 1 reply; 12+ messages in thread
From: Hao Ge @ 2024-12-05 2:14 UTC (permalink / raw)
To: Suren Baghdasaryan; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
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.
Thanks
Best regards
Hao
>>
>>>> }
>>>>
>>>> return 0;
>>>> --
>>>> 2.25.1
>>>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-05 2:14 ` Hao Ge
@ 2024-12-05 3:20 ` Hao Ge
2024-12-05 14:48 ` Suren Baghdasaryan
0 siblings, 1 reply; 12+ messages in thread
From: Hao Ge @ 2024-12-05 3:20 UTC (permalink / raw)
To: Suren Baghdasaryan; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
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.
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.
Based on that, I would suggest sticking with the V2 version.
Thanks
Best Regards
Hao
>
>
> Thanks
>
> Best regards
>
> Hao
>
>
>>>
>>>>> }
>>>>>
>>>>> return 0;
>>>>> --
>>>>> 2.25.1
>>>>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-05 3:20 ` Hao Ge
@ 2024-12-05 14:48 ` Suren Baghdasaryan
2024-12-05 15:34 ` Hao Ge
0 siblings, 1 reply; 12+ messages in thread
From: Suren Baghdasaryan @ 2024-12-05 14:48 UTC (permalink / raw)
To: Hao Ge; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
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?
>
> Based on that, I would suggest sticking with the V2 version.
>
>
> Thanks
>
> Best Regards
>
> Hao
>
> >
> >
> > Thanks
> >
> > Best regards
> >
> > Hao
> >
> >
> >>>
> >>>>> }
> >>>>>
> >>>>> return 0;
> >>>>> --
> >>>>> 2.25.1
> >>>>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-05 14:48 ` Suren Baghdasaryan
@ 2024-12-05 15:34 ` Hao Ge
2024-12-05 16:25 ` Hao Ge
0 siblings, 1 reply; 12+ messages in thread
From: Hao Ge @ 2024-12-05 15:34 UTC (permalink / raw)
To: Suren Baghdasaryan; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
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
>>>>>>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-05 15:34 ` Hao Ge
@ 2024-12-05 16:25 ` Hao Ge
2024-12-05 17:05 ` [PATCH v3] " Hao Ge
0 siblings, 1 reply; 12+ messages in thread
From: Hao Ge @ 2024-12-05 16:25 UTC (permalink / raw)
To: Suren Baghdasaryan; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
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
>>>>>>>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-05 16:25 ` Hao Ge
@ 2024-12-05 17:05 ` Hao Ge
2024-12-06 19:03 ` Suren Baghdasaryan
0 siblings, 1 reply; 12+ messages in thread
From: Hao Ge @ 2024-12-05 17:05 UTC (permalink / raw)
To: surenb, kent.overstreet, akpm; +Cc: linux-mm, linux-kernel, hao.ge, Hao Ge
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.
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>
---
v3: Based on Suren's suggestion, I modified the code,Thank you for Suren.
I realized that the 'poisoned' is actually not needed, so I removed it
Due to these changes, update the commit message.
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 | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
index 4ee6caa6d2da..f942408b53ef 100644
--- a/lib/alloc_tag.c
+++ b/lib/alloc_tag.c
@@ -424,6 +424,15 @@ static int vm_module_tags_populate(void)
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,
+ new_end - module_tags.start_addr,
+ KASAN_VMALLOC_PROT_NORMAL);
+
return 0;
}
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic
2024-12-05 17:05 ` [PATCH v3] " Hao Ge
@ 2024-12-06 19:03 ` Suren Baghdasaryan
0 siblings, 0 replies; 12+ messages in thread
From: Suren Baghdasaryan @ 2024-12-06 19:03 UTC (permalink / raw)
To: Hao Ge; +Cc: kent.overstreet, akpm, linux-mm, linux-kernel, Hao Ge
On Thu, Dec 5, 2024 at 9:05 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.
>
> 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>
Acked-by: Suren Baghdasaryan <surenb@google.com>
> ---
> v3: Based on Suren's suggestion, I modified the code,Thank you for Suren.
> I realized that the 'poisoned' is actually not needed, so I removed it
> Due to these changes, update the commit message.
>
> 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 | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> index 4ee6caa6d2da..f942408b53ef 100644
> --- a/lib/alloc_tag.c
> +++ b/lib/alloc_tag.c
> @@ -424,6 +424,15 @@ static int vm_module_tags_populate(void)
> 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,
> + new_end - module_tags.start_addr,
> + KASAN_VMALLOC_PROT_NORMAL);
> +
> return 0;
> }
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-12-06 19:03 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-04 7:52 [PATCH] mm/alloc_tag: fix vm_module_tags_populate's KASAN poisoning logic Hao Ge
2024-12-04 8:34 ` [PATCH v2] " Hao Ge
2024-12-04 14:39 ` Suren Baghdasaryan
2024-12-04 15:07 ` Hao Ge
2024-12-04 19:33 ` Suren Baghdasaryan
2024-12-05 2:14 ` Hao Ge
2024-12-05 3:20 ` Hao Ge
2024-12-05 14:48 ` Suren Baghdasaryan
2024-12-05 15:34 ` Hao Ge
2024-12-05 16:25 ` Hao Ge
2024-12-05 17:05 ` [PATCH v3] " Hao Ge
2024-12-06 19:03 ` Suren Baghdasaryan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox