linux-kernel - Re: [PATCHv7 3/7] kho: drop notifiers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+CK2bB_xPAsHXU62Hd5iBt-+Jf2BiXQM4M-QEL=AA_Xu-APhg@mail.gmail.com>
Date: Fri, 24 Oct 2025 12:15:32 -0400
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Pratyush Yadav <pratyush@...nel.org>
Cc: akpm@...ux-foundation.org, brauner@...nel.org, corbet@....net, 
	graf@...zon.com, jgg@...pe.ca, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-mm@...ck.org, masahiroy@...nel.org, 
	ojeda@...nel.org, rdunlap@...radead.org, rppt@...nel.org, tj@...nel.org
Subject: Re: [PATCHv7 3/7] kho: drop notifiers

On Fri, Oct 24, 2025 at 11:52 AM Pratyush Yadav <pratyush@...nel.org> wrote:
>
> On Fri, Oct 24 2025, Pratyush Yadav wrote:
>
> > On Fri, Oct 24 2025, Pasha Tatashin wrote:
> >
> >>> > -int kho_add_subtree(struct kho_serialization *ser, const char *name, void *fdt)
> >>> > +int kho_add_subtree(const char *name, void *fdt)
> >>> >  {
> >>> > -     int err = 0;
> >>> > -     u64 phys = (u64)virt_to_phys(fdt);
> >>> > -     void *root = page_to_virt(ser->fdt);
> >>> > +     struct kho_sub_fdt *sub_fdt;
> >>> > +     int err;
> >>> >
> >>> > -     err |= fdt_begin_node(root, name);
> >>> > -     err |= fdt_property(root, PROP_SUB_FDT, &phys, sizeof(phys));
> >>> > -     err |= fdt_end_node(root);
> >>> > +     sub_fdt = kmalloc(sizeof(*sub_fdt), GFP_KERNEL);
> >>> > +     if (!sub_fdt)
> >>> > +             return -ENOMEM;
> >>> >
> >>> > -     if (err)
> >>> > -             return err;
> >>> > +     INIT_LIST_HEAD(&sub_fdt->l);
> >>> > +     sub_fdt->name = name;
> >>> > +     sub_fdt->fdt = fdt;
> >>> >
> >>> > -     return kho_debugfs_fdt_add(&kho_out.dbg, name, fdt, false);
> >>> > +     mutex_lock(&kho_out.fdts_lock);
> >>> > +     list_add_tail(&sub_fdt->l, &kho_out.sub_fdts);
> >>> > +     err = kho_debugfs_fdt_add(&kho_out.dbg, name, fdt, false);
> >>>
> >>> I think you should remove sub_fdt from the list and kfree() it on error
> >>> here. Otherwise we signal an error to the caller and they might free
> >>> sub_fdt->fdt, which will later result in a use-after-free at
> >>> __kho_finalize().
> >>
> >> I think, it is better to simply do:
> >> WARN_ON_ONCE(kho_debugfs_fdt_add(...));
> >> Now debugfs is optional, and there is no reason to return an error to
> >> a caller if kho_debugfs_fdt_add() fails
> >
> > Yeah, that works too.
>
> On a second thought, maybe pr_warn() instead of WARN_ON()? This isn't an
> assertion since the debugfs creation can fail for many reasons. It isn't
> expected to always succeed. So a full WARN_ON() splat seems overkill.

I sent it with WARN_ON_ONCE(), I can change it to pr_warn_once() if
there is another revision, otherwise we can just send a separate patch
to make the change it is not that important.

Pasha

>
> [...]
>
> --
> Regards,
> Pratyush Yadav