Message-ID: <CAOOmCE8X=VK7hjAB48Jp2x4-5GhtSJKYHB9scdN9CL6gZKvMJQ@mail.gmail.com>
Date:   Fri, 28 Apr 2023 15:46:41 -0500
From:   Jorge Lopez <jorgealtxwork@...il.com>
To:     Thomas Weißschuh <thomas@...ch.de>
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 Fri, Apr 28, 2023 at 11:06 AM Thomas Weißschuh <thomas@...ch.de> wrote:
>
> On 2023-04-28 10:40:59-0500, Jorge Lopez wrote:
> > On Fri, Apr 28, 2023 at 10:21 AM Thomas Weißschuh <thomas@...ch.de> wrote:
> > >
> > > On 2023-04-28 09:58:01-0500, Jorge Lopez wrote:
> > > > On Fri, Apr 28, 2023 at 1:03 AM Thomas Weißschuh <thomas@...ch.de> wrote:
> > > > >
> > > > > 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
> > >
> > > <snip>
> > >
> > > > > > > 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.
> > > >
> > > > BIOS stores a max number of audit logs appropriate to the current
> > > > audit log size.The first audit logs are kept in a FIFO queue by BIOS
> > > > so when the queue is full and a new audit log arrives, then the  first
> > > > audit log will be deleted.
> > >
> > > How does it determine "appropriate"?
> > > This would also be great in a comment.
> > >
> > > If the BIOS is just using FIFO the driver could return the first
> > > LOG_MAX_ENTRIES entries.
> > > This would avoid trusting the firmware for a reasonable definition of
> > > "appropriate".
> > >
> > > > >
> > > > > > > > +
> > > > > > > > +     if (ret < 0)
> > > > > > > > +             return ret;
> > > > >
> > > > > And this should first validate ret and then count.
> > > >
> > > > Done!
> > > >
> > > > >
> > > > > > > > +
> > > > > > > > +     /*
> > > > > > > > +      * 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 need a comment.
> > > >
> > > > Done!
> > > >
> > > > >
> > > > > > >
> > > > > > > > +             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.
> > > > >
> > > > BIOS currently stores 16 bytes for each audit log although the WMI
> > > > query reads 128 bytes.  The 128 bytes size is set to provide support
> > > > in future BIOS for audit log sizes >= 16 and < 128 bytes.
> > >
> > > And if an old driver is running on a new BIOS then this would write out
> > > of bounds.
> > > Or if the BIOS is buggy.
> > >
> > > If the current driver can only handle 16 byte sized log entries then the
> > > this should be used in the call to HPWMI_SURESTART_GET_LOG.
> >
> > BIOS WMI specification indicates that the HPWMI_SURESTART_GET_LOG call
> > expects a 128 byte size output buffer regardless of the actual audit
> > log size currently supported.
> >
> > Return Values:
> > Byte 0-15: a requested Audit Log entry (Each Audit log is 16 bytes)
> > Byte 16-127: Unused
> > >
> > > Storing it in a 128 byte stackvariable would also sidestep the issue.
> >
> > The driver hardcodes the audit log size to 16 bytes.  If the new BIOS
> > provides an audit log that is larger than 16 bytes, then the logs
> > provided to the user application by the old driver will be truncated.
>
> HPWMI_SURESTART_GET_LOG is directly passed a pointer into "buf" which
> comes from sysfs core and is one page, 4096 bytes large.
> It is told to write 128 bytes into it at a given offset.
>
> In the loop if i == 253 then this offset will be LOG_ENTRY_SIZE * 253 = 4048.
>
> So on a new BIOS the driver may write 128 bytes at offset 4048.
> This goes up to 4175 which is larger than the 4096 buffer.
>
> (See also the calculation in the previous mail)
>
> Just use a 128 byte stack buffer and copy 16 bytes of it to the output
> buffer.
> (After having validated that the BIOS actually returned 16 bytes)

Thank you for the clarification.  Done!

Powered by blists - more mailing lists