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

["Thomas Weißschuh" <thomas@t-8ch.de>]

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@t-8ch.de> 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.

> > 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.

> > > +{
> > > +     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.

> >
> > > +     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().

> >
> > > +     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.

> >
> > > + */
> > > +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?

> 
> > > +     return p;
> > > +}
> > > +
> > > +/*
> >
> > kernel-doc comments start with "/**". Note the two asterisks.
> Done

This also needs to be done all over the driver.