linux-kernel - Re: [PATCH v11 12/14] HP BIOSCFG driver

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: <479b18e3-a35b-45c7-8c8a-cd30af646977@t-8ch.de>
Date:   Fri, 28 Apr 2023 08:03:56 +0200
From:   Thomas Weißschuh <thomas@...ch.de>
To:     Jorge Lopez <jorgealtxwork@...il.com>
Cc:     hdegoede@...hat.com, platform-driver-x86@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 12/14] HP BIOSCFG driver - surestart-attributes

On 2023-04-27 17:17:57-0500, Jorge Lopez wrote:
> On Sun, Apr 23, 2023 at 7:16 AM Thomas Weißschuh <thomas@...ch.de> wrote:
> >
> > On 2023-04-20 11:54:52-0500, Jorge Lopez wrote:
> > >  .../x86/hp/hp-bioscfg/surestart-attributes.c  | 130 ++++++++++++++++++
> > >  1 file changed, 130 insertions(+)
> > >  create mode 100644 drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c
> > >
> > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c
> > > new file mode 100644
> > > index 000000000000..72952758ffe3
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c
> > > @@ -0,0 +1,130 @@

<snip>

> > > +
> > > +/*
> > > + * audit_log_entries_show() - Return all entries found in log file
> > > + */
> > > +static ssize_t audit_log_entries_show(struct kobject *kobj,
> > > +                                   struct kobj_attribute *attr, char *buf)
> > > +{
> > > +     int ret;
> > > +     int i;
> > > +     u32 count = 0;
> > > +
> > > +     // Get the number of event logs
> > > +     ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT,
> > > +                                HPWMI_SURESTART,
> > > +                                &count, 1, sizeof(count));
> > > +
> > > +     /*
> > > +      * The show() api will not work if the audit logs ever go
> > > +      *  beyond 4KB
> > > +      */
> > > +     if (count * LOG_ENTRY_SIZE > PAGE_SIZE)
> > > +             return -EFAULT;
> >
> > The error code seems not to match.
> >
> 
> Changing error to -EINVAL

-EIO seems better.

The problem is not due to some value a user passed but an unhandled from
the hardware.

> 
> > Instead of not returning any data, why not show as many results as
> > possible?
> >
> 
> if count * LOG_ENTRY_SIZE > PAGE_SIZE then I prefer to return an error.
> if the count is correct but a failure occurs while reading individual
> audit logs then we will return a partial list of all audit logs
> This changes will be included in Version 12

What prevents the firmware from having more log entries?
Wouldn't these audit log entries not accumulate for each logged
operation over the lifetime of the device / boot?

This would make the interface unusable as soon as there are more
entries.

> > > +
> > > +     if (ret < 0)
> > > +             return ret;

And this should first validate ret and then count.

> > > +
> > > +     /*
> > > +      * We are guaranteed the buffer is 4KB so today all the event
> > > +      * logs will fit
> > > +      */
> > > +
> > > +     for (i = 0; ((i < count) & (ret >= 0)); i++) {
> >
> > &&
> >
> > Better yet, pull the condition ret >= 0 into the body, as an else-branch
> > for the existing check.
> >
> 
> Done!
> 
> > > +             *buf = (i + 1);
> >
> > Isn't this directly overwritten by the query below?
> 
> buf input value indicates the audit log to be read hence the reason
> why it is overwritten.
> This is an expected behavior.

So this is read by the HPWMI_SURESTART_GET_LOG method in the firmware?

Make sense but needs a comment.

> >
> > > +             ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG,
> > > +                                        HPWMI_SURESTART,
> > > +                                        buf, 1, 128);
> > > +             if (ret >= 0)
> > > +                     buf += LOG_ENTRY_SIZE;
> >
> > So 128 bytes are read but only the first 16 bytes are preserved?
> >
> > The documentation says that each entry has 128 bytes in the file.
> > And that they are separated by ";", which is not implemented.
> 
> The statement will be removed from documentation  (separated by ";")
> audit log size is 16 bytes.
> >
> > Can the audit-log not contain all-zero bytes?
> > If it does this would need to be a bin_attribute.
> 
> Bytes 16-127 are ignored and not used at this time.  If the audit log
> changes, then the driver will need to change to accommodate the new
> audit log size.

buf is not guaranteed to have 128 bytes left for this data.

For example if this is entry number 253 we are at offset 253 * 16 = 4048
in the sysfs buffer. Now hw_wmi_perform_query may try to write to 4048 +
127 = 4175 which is out of bounds for the buf of size 4096.

Writing first to a stack buffer would be better,
or pass outsize = LOG_ENTRY_SIZE.

> The audit log file cannot contain all zero bytes.

I doublechecked this and zero bytes seem to also be fine in normal text
attributes.

> > > +   return (count * LOG_ENTRY_SIZE);

If one of the calls to hp_wmi_perform_query() fails this return value is wrong,
it does not reflect the amount of actually written data.