Message-ID: <CAJuCfpF8ftPcd24y2L4C_37GDMC4iaDDh8SicwspwmVAkB=3fQ@mail.gmail.com>
Date: Thu, 5 Dec 2024 06:48:38 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: Hao Ge <hao.ge@...ux.dev>
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

On Wed, Dec 4, 2024 at 7:20 PM Hao Ge <hao.ge@...ux.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@...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.
>
> 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
> >>>>>

Powered by blists - more mailing lists