linux-kernel - Re: [PATCH v1] ACPI: bus: Adjust acpi_osc

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJZ5v0jCyRLpEA0Fmbjx8cNuden3sgqWvVO6_wmvCTHXVwb1LA@mail.gmail.com>
Date: Fri, 2 Jan 2026 13:06:48 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux ACPI <linux-acpi@...r.kernel.org>, 
	Dan Carpenter <dan.carpenter@...aro.org>, LKML <linux-kernel@...r.kernel.org>, 
	Linux PCI <linux-pci@...r.kernel.org>, Bjorn Helgaas <helgaas@...nel.org>, 
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>, Hans de Goede <hansg@...nel.org>, 
	Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v1] ACPI: bus: Adjust acpi_osc_handshake() parameter list

On Fri, Jan 2, 2026 at 12:53 PM Jonathan Cameron
<jonathan.cameron@...wei.com> wrote:
>
> On Fri, 26 Dec 2025 14:48:45 +0100
> "Rafael J. Wysocki" <rafael@...nel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > For the sake of interface cleanliness, it is better to avoid using
> > ACPICA data types in the parameter lists of helper functions that
> > don't belong to ACPICA, so adjust the parameter list of recently
> > introduced acpi_osc_handshake() to take a capabilities buffer pointer
> > and the size of the buffer (in u32 size units) as parameters directly
> > instead of a struct acpi_buffer pointer.
> >
> > This is also somewhat more straightforward on the caller side because
> > they won't need to create struct acpi_buffer objects themselves to pass
> > them to the helper function and it guarantees that the size of the
> > buffer in bytes will always be a multiple of 4 (the size of u32).
> >
> > Moreover, it addresses a premature cap pointer dereference and
> > eliminates a sizeof(32) that should have been sizeof(u32) [1].
> >
> > Fixes: e5322888e6bf ("ACPI: bus: Rework the handling of \_SB._OSC platform features")
> > Reported-by: kernel test robot <lkp@...el.com>
> > Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> > Closes: https://lore.kernel.org/linux-acpi/202512242052.W4GhDauV-lkp@intel.com/
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> A couple of minor comments inline.  I see you have it queued up already,

Yeah, mostly to address build warnings in linux-next.

> but FWIW nothing here major enough to warrant reverting that.
> Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.com>
>
> > ---
> >  drivers/acpi/bus.c |   30 ++++++++++++------------------
> >  1 file changed, 12 insertions(+), 18 deletions(-)
> >
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -326,31 +326,33 @@ out:
> >  EXPORT_SYMBOL(acpi_run_osc);
> >
> >  static int acpi_osc_handshake(acpi_handle handle, const char *uuid_str,
> > -                           int rev, struct acpi_buffer *cap)
> > +                           int rev, u32 *capbuf, size_t bufsize)
>
> Size parameters in number of u32s is to me a little confusing but
> I guess this is only used locally so that's probably fine.
> I'd have been tempted to call it dwords or something like that.

Well, this technically is the number of elements in the capbuf[]
array, so I guess it could be called capbuf_size or something like
that.

> >  {
> >       union acpi_object in_params[4], *out_obj;
> > -     size_t bufsize = cap->length / sizeof(u32);
> >       struct acpi_object_list input;
> > +     struct acpi_buffer cap = {
> > +             .pointer = capbuf,
> > +             .length = bufsize * sizeof(32),
>
> You fixed this up already but just for completeness.
> sizeof(u32)

Yup.

> > +     };
> >       struct acpi_buffer output;
> > -     u32 *capbuf, *retbuf, test;
> > +     u32 *retbuf, test;
> >       guid_t guid;
> >       int ret, i;
> >
> > -     if (!cap || cap->length < 2 * sizeof(32) || guid_parse(uuid_str, &guid))
> > +     if (!capbuf || bufsize < 2 || guid_parse(uuid_str, &guid))
> >               return -EINVAL;
> >
> >       /* First evaluate _OSC with OSC_QUERY_ENABLE set. */
> > -     capbuf = cap->pointer;
> >       capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
> >
> > -     ret = acpi_eval_osc(handle, &guid, rev, cap, in_params, &output);
> > +     ret = acpi_eval_osc(handle, &guid, rev, &cap, in_params, &output);
> >       if (ret)
> >               return ret;
> >
> >       out_obj = output.pointer;
> >       retbuf = (u32 *)out_obj->buffer.pointer;
> >
> > -     if (acpi_osc_error_check(handle, &guid, rev, cap, retbuf)) {
> > +     if (acpi_osc_error_check(handle, &guid, rev, &cap, retbuf)) {
> >               ret = -ENODATA;
> >               goto out;
> >       }
> > @@ -403,7 +405,7 @@ static int acpi_osc_handshake(acpi_handl
> >                */
> >               acpi_handle_err(handle, "_OSC: errors while processing control request\n");
> >               acpi_handle_err(handle, "_OSC: some features may be missing\n");
> > -             acpi_osc_error_check(handle, &guid, rev, cap, retbuf);
> > +             acpi_osc_error_check(handle, &guid, rev, &cap, retbuf);
> >       }
> >
> >  out:
> > @@ -446,10 +448,6 @@ static void acpi_bus_osc_negotiate_platf
> >  {
> >       static const u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
> >       u32 capbuf[2], feature_mask;
> > -     struct acpi_buffer cap = {
> > -             .pointer = capbuf,
> > -             .length = sizeof(capbuf),
> > -     };
> >       acpi_handle handle;
> >
> >       feature_mask = OSC_SB_PR3_SUPPORT | OSC_SB_HOTPLUG_OST_SUPPORT |
> > @@ -497,7 +495,7 @@ static void acpi_bus_osc_negotiate_platf
> >
> >       acpi_handle_info(handle, "platform _OSC: OS support mask [%08x]\n", feature_mask);
> >
> > -     if (acpi_osc_handshake(handle, sb_uuid_str, 1, &cap))
> > +     if (acpi_osc_handshake(handle, sb_uuid_str, 1, capbuf, 2))
>
> As below. Maybe ARRAY_SIZE(capbuf) instead of that 2.

That's a good idea, I'll change it in the patch.

> >               return;
> >
> >       feature_mask = capbuf[OSC_SUPPORT_DWORD];
> > @@ -532,10 +530,6 @@ static void acpi_bus_osc_negotiate_usb_c
> >  {
> >       static const u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
> >       u32 capbuf[3], control;
> > -     struct acpi_buffer cap = {
> > -             .pointer = capbuf,
> > -             .length = sizeof(capbuf),
> > -     };
> >       acpi_handle handle;
> >
> >       if (!osc_sb_native_usb4_support_confirmed)
> > @@ -550,7 +544,7 @@ static void acpi_bus_osc_negotiate_usb_c
> >       capbuf[OSC_SUPPORT_DWORD] = 0;
> >       capbuf[OSC_CONTROL_DWORD] = control;
> >
> > -     if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, &cap))
> > +     if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, capbuf, 3))
>
> Maybe ARRAY_SIZE(capbuf) just to avoid any chance they get out of sync?
>
> >               return;
> >
> >       osc_sb_native_usb4_control = capbuf[OSC_CONTROL_DWORD];
> >
> >
> >
> >

Thanks!