Re: [PATCH v11 12/14] HP BIOSCFG driver - surestart-attributes

[Thomas Weißschuh]

On 2023-04-28 10:40:59-0500, Jorge Lopez wrote:
> On Fri, Apr 28, 2023 at 10:21 AM Thomas Weißschuh <thomas@xxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxx> 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)