| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dccv6plsmlqd6gabd5aaor4h7wnp6k6nnjudkjfsvzk7tv5yc2@prn3jcmug2xw>
Date: Tue, 14 Jan 2025 21:45:18 -0500
From: Kurt Borja <kuurtb@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: platform-driver-x86@...r.kernel.org,
"Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, Mario Limonciello <mario.limonciello@....com>,
Armin Wolf <W_Armin@....de>, Joshua Grisham <josh@...huagrisham.com>,
"Derek J. Clark" <derekjohn.clark@...il.com>, Hans de Goede <hdegoede@...hat.com>,
Maximilian Luz <luzmaximilian@...il.com>, "Lee, Chun-Yi" <jlee@...e.com>,
Shyam Sundar S K <Shyam-sundar.S-k@....com>, Corentin Chary <corentin.chary@...il.com>,
"Luke D. Jones" <luke@...nes.dev>, Lyndon Sanche <lsanche@...deno.ca>,
Ike Panhc <ike.pan@...onical.com>, Henrique de Moraes Holschuh <hmh@....eng.br>,
Mark Pearson <mpearson-lenovo@...ebb.ca>, Alexis Belmonte <alexbelm48@...il.com>,
Ai Chao <aichao@...inos.cn>, Gergo Koteles <soyer@....hu>, Dell.Client.Kernel@...l.com,
ibm-acpi-devel@...ts.sourceforge.net
Subject: Re: [PATCH v2 15/18] ACPI: platform_profile: Remove
platform_profile_handler from exported symbols
On Tue, Jan 14, 2025 at 07:55:53PM +0200, Ilpo Järvinen wrote:
> On Tue, 14 Jan 2025, Kurt Borja wrote:
> > On Tue, Jan 14, 2025 at 06:55:34PM +0200, Ilpo Järvinen wrote:
> > > On Tue, 14 Jan 2025, Kurt Borja wrote:
> > >
> > > > In order to protect the platform_profile_handler from API consumers,
> > > > allocate it in platform_profile_register() and modify it's signature
> > > > accordingly.
> > > >
> > > > Remove the platform_profile_handler from all consumer drivers and
> > > > replace them with a pointer to the class device, which is
> > > > now returned from platform_profile_register().
> > > >
> > > > Replace *pprof with a pointer to the class device in the rest of
> > > > exported symbols.
> > > >
> > > > Signed-off-by: Kurt Borja <kuurtb@...il.com>
> > > > ---
>
> > > > -int platform_profile_register(struct platform_profile_handler *pprof, void *drvdata)
> > > > +struct device *platform_profile_register(struct device *dev, const char *name,
> > > > + void *drvdata,
> > > > + const struct platform_profile_ops *ops)
> > > > {
> > > > + int minor;
> > > > int err;
> > > >
> > > > - /* Sanity check the profile handler */
> > > > - if (!pprof || !pprof->ops->profile_set || !pprof->ops->profile_get ||
> > > > - !pprof->ops->probe) {
> > > > + /* Sanity check */
> > > > + if (!dev || !name || !ops || !ops->profile_get ||
> > > > + !ops->profile_set || !ops->probe) {
> > > > pr_err("platform_profile: handler is invalid\n");
> > > > - return -EINVAL;
> > > > + return ERR_PTR(-EINVAL);
> > > > }
> > > >
> > > > - err = pprof->ops->probe(drvdata, pprof->choices);
> > > > + struct platform_profile_handler *pprof __free(kfree) = kzalloc(
> > > > + sizeof(*pprof), GFP_KERNEL);
> > > > + if (!pprof)
> > > > + return ERR_PTR(-ENOMEM);
> > > > +
> > > > + err = ops->probe(drvdata, pprof->choices);
> > > > if (err < 0)
> > > > - return err;
> > > > + return ERR_PTR(err);
> > > >
> > > > if (bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST)) {
> > > > pr_err("platform_profile: no available profiles\n");
> > > > - return -EINVAL;
> > > > + return ERR_PTR(-EINVAL);
> > > > }
> > > >
> > > > guard(mutex)(&profile_lock);
> > > >
> > > > /* create class interface for individual handler */
> > > > - pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> > > > - if (pprof->minor < 0)
> > > > - return pprof->minor;
> > > > + minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> > > > + if (minor < 0)
> > > > + return ERR_PTR(minor);
> > > >
> > > > + pprof->name = name;
> > > > + pprof->ops = ops;
> > > > + pprof->minor = minor;
> > > > pprof->class_dev.class = &platform_profile_class;
> > > > - pprof->class_dev.parent = pprof->dev;
> > > > + pprof->class_dev.parent = dev;
> > > > dev_set_drvdata(&pprof->class_dev, drvdata);
> > > > dev_set_name(&pprof->class_dev, "platform-profile-%d", pprof->minor);
> > > > err = device_register(&pprof->class_dev);
> > > > if (err) {
> > > > - put_device(&pprof->class_dev);
> > > > + put_device(&no_free_ptr(pprof)->class_dev);
> > > > goto cleanup_ida;
> > > > }
> > > >
> > > > @@ -504,20 +524,21 @@ int platform_profile_register(struct platform_profile_handler *pprof, void *drvd
> > > > if (err)
> > > > goto cleanup_cur;
> > > >
> > > > - return 0;
> > > > + return &no_free_ptr(pprof)->class_dev;
> > > >
> > > > cleanup_cur:
> > > > - device_unregister(&pprof->class_dev);
> > > > + device_unregister(&no_free_ptr(pprof)->class_dev);
> > >
> > > I don't like how this is architected.
> > >
> > > IMO, no_free_ptr() should not appear on error/rollback paths. The pointer
> > > is going to be freed despite the code just told it's not going to be
> > > freed, which sends conflicting signals. Obviously, it is because this
> > > function has relinquished its ownership of the pointer but as is it seems
> > > a dangerous/confusing pattern.
> >
> > Makes sense.
> >
> > Quick fix would be to replace `goto cleanup_cur` with
> >
> > device_unregister(&no_free_ptr(pprof)->class_dev);
> > goto cleanup_ida;
> >
> > and add a comment about ownership. Similar to the put_device() call
> > above. Is this ok? If not I will think of a better way of writing this.
>
> I think it would still be on the error path which is undesirable. While a
> comment would make it understandable, it would be more logical to call
> no_free_ptr() near device_register() which is when the ownership
> gets transferred.
>
> The trouble with that approach then is that no_free_ptr(pprof) will set
> the pprof to NULL because of how the internal cleanup magic prevents
> automatic freeing of pprof (Don't ask me how I know about that trap :-D).
:')
>
> I suppose you could take pointer of the pprof->class_dev into a local
> variable before making the device_register() call since that is all you
> need after that point?
>
> So my suggestion is along the lines of:
>
> /* device_register() takes the ownership of the pointer */
> class_dev = &no_free_ptr(pprof)->class_dev;
> err = device_register(class_dev);
> ...
Yes, this makes a lot of sense. I will do it this way.
Thanks Ilpo.
~ Kurt
>
>
> > > > cleanup_ida:
> > > > - ida_free(&platform_profile_ida, pprof->minor);
> > > > + ida_free(&platform_profile_ida, minor);
> > > >
> > > > - return err;
> > > > + return ERR_PTR(err);
> > > > }
> > > > EXPORT_SYMBOL_GPL(platform_profile_register);
>
> --
> i.
>
Powered by blists - more mailing lists