Re: [PATCH v11 02/14] HP BIOSCFG driver - biosattr-interface

[Jorge Lopez]

Hi Thomas,

On Mon, Apr 24, 2023 at 4:04 PM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> On 2023-04-24 15:33:26-0500, Jorge Lopez wrote:
> > Hi Thomas,
> >
> > Please see my comments below.
> >
> > On Sat, Apr 22, 2023 at 4:30 PM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
> > >
> > > Hi Jorge,
> > >
> > > checkpatch.pl finds some issues on your patches.
> > > Please make sure checkpath.pl --strict is happy.
> > >
> > I wasn't aware of the '--strict' parameter.  It is not part of the
> > help information for checkpath.pl tool.
> > Nonetheless, I will use it forward.
> > Thanks
>
> It's an alias to --subjective. But indeed, it's hard to see in the help
> output.
Thanks
>
> > > On 2023-04-20 11:54:42-0500, Jorge Lopez wrote:
> > > > ---
> > > > Based on the latest platform-drivers-x86.git/for-next
> > > No need to initialize auth_token_choice and start.
> > > Consider coalescing variable declaration to avoid wasting vertical
> > > space.
> > >
> > Done!
>
> Please note that this affects many parts of the driver,
> try to fix it everywhere.

It will be done across all files

>
> > > > +{
> > > > +     struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };
> > > > +     struct bios_return *bios_return;
> > > > +     union acpi_object *obj = NULL;
> > > > +     struct bios_args *args = NULL;
> > > > +     int mid, actual_outsize;
> > > > +     size_t bios_args_size;
> > > > +     int ret;
> > > > +
> > > > +     mid = encode_outsize_for_pvsz(outsize);
> > > > +     if (WARN_ON(mid < 0))
> > > > +             return mid;
> > > > +
> > > > +     bios_args_size = struct_size(args, data, insize);
> > > > +     args = kmalloc(bios_args_size, GFP_KERNEL);
> > > > +     if (!args)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     input.length = bios_args_size;
> > > > +     input.pointer = args;
> > > > +
> > > > +     args->signature = 0x55434553;
> > >
> > > What does this number mean?
> > This is a HEX representation of the word  'SECU' expected by BIOS as a signa.
>
> Sounds like a good thing to comment or put into a #define.

I will add a comment since it is only used here.
>
> > >
> > > > +     args->command = command;
> > > > +     args->commandtype = query;
> > > > +     args->datasize = insize;
> > > > +     memcpy(args->data, buffer, flex_array_size(args, data, insize));
> > > > +
> > > > +     ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output);
> > >
> > > The driver is mixing calls to the UUID based APIs and the wmi_device
> > > ones.
> > > wmi_devices is newer and preferred.
> >
> > The driver  calls wmi_evaluate_method when initiating an WMI call.
> > Where is the driver mixing calls to the UUID based APIs and the
> > wmi_device one?
> > WMI calls are made by calling hp_wmi_perform_query() which invokes
> > wmi_evaluate_method().
> > Did I miss something?
>
> wmi_evaluate_method() is UUID-based.
> struct wmi_driver is wmi_device based.
>
> The wmi_driver/wmi_device code essentially does nothing and is only used
> to validate that a device is present.
> The same can be done more easily wmi_has_guid().
>

Thank you for the clarification.
> > >
> > > > +     bioscfg_wmi_error_and_message(ret);
> > > > +
> > > > +     if (ret)
> > > > +             goto out_free;
> > > > +
> > > > +     obj = output.pointer;
> > > > +     if (!obj) {
> > > > +             ret = -EINVAL;
> > > > +             goto out_free;
> > > > +     }
> > > > +     if (query != HPWMI_SECUREPLATFORM_GET_STATE &&
> > > > +         command != HPWMI_SECUREPLATFORM)
> > > > +             if (obj->type != ACPI_TYPE_BUFFER ||
> > > > +                 obj->buffer.length < sizeof(*bios_return)) {
> > > > +                     pr_warn("query 0x%x returned wrong type or too small buffer\n", query);
> > > > +                     ret = -EINVAL;
> > > > +                     goto out_free;
> > > > +             }
> > > > +
> > > > +
> > > > +     bios_return = (struct bios_return *)obj->buffer.pointer;
> > >
> > > For query == HPWMI_SECUREPLATFORM_GET_STATE && command == HPWMI_SECUREPLATFORM
> > > this is not guaranteed to be a buffer.
> >
> > BIOS ensures the output is of buffer type and  buffer of 1024 bytes in
> > size.  This check also help us validate that BIOS only returns a
> > buffer type for this query/command type.
>
> The kernel does not trust the BIOS :-)
> It trusts nothing and nobody.
>
> All cases should be validated.

Additional validation will be added to cover all cases.

>
> > >
> > > > + */
> > > > +void *ascii_to_utf16_unicode(u16 *p, const u8 *str)
> > > > +{
> > > > +     int len = strlen(str);
> > > > +     int ret;
> > > > +
> > > > +     /*
> > > > +      * Add null character when reading an empty string
> > > > +      * "02 00 00 00"
> > > > +      */
> > > > +     if (len == 0)
> > > > +             return utf16_empty_string(p);
> > > > +
> > > > +     /* Move pointer len * 2 number of bytes */
> > > > +     *p++ = len * 2;
> > > > +     ret = utf8s_to_utf16s(str, strlen(str), UTF16_HOST_ENDIAN, p, len);
> > > > +     if (ret < 0) {
> > > > +             dev_err(bioscfg_drv.class_dev, "UTF16 conversion failed\n");
> > > > +             goto ascii_to_utf16_unicode_out;
> > > > +     }
> > >
> > > What if ret != len ?
> >
> > only in conditions where utf8s_to_utf16s an error, we can state  ret != len.
> > ret == len when utf8s_to_utf16s() is successful.
> > >
> > > > +
> > > > +     if ((ret * sizeof(u16)) > U16_MAX) {
> > > > +             dev_err(bioscfg_drv.class_dev, "Error string too long\n");
> > > > +             goto ascii_to_utf16_unicode_out;
> > > > +     }
> > > > +
> > > > +ascii_to_utf16_unicode_out:
> > > > +     p += len;
> > >
> > > In cases of errors this will still point to the end of the data that
> > > should have been written but was not actually written.
> > > The caller has no way to recognize the error case.
> > >
> > That is correct.  If an error occurs, we only provide an error message
> > for those conditions.
>
> But the caller has no idea that this error occurred and will try to use
> the garbage buffer.
> The error should be communicated to the caller, and the caller has to
> validate the result.
> Maybe return NULL?

returning NULL will be a good option so I will review what the impact
will be across the driver
>
> >
> > > > +     return p;
> > > > +}
> > > > +
> > > > +/*
> > >
> > > kernel-doc comments start with "/**". Note the two asterisks.
> > Done
>
> This also needs to be done all over the driver.

It will be done across all files.