On Fri, May 12, 2023 at 9:12 AM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, 12 May 2023, Jorge Lopez wrote:
>
> > On Thu, May 11, 2023 at 4:32 AM Ilpo Järvinen
> > <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, 10 May 2023, Jorge Lopez wrote:
> > >
> > > > On Tue, May 9, 2023 at 8:57 AM Ilpo Järvinen
> > > > <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 5 May 2023, Jorge Lopez wrote:
> > > > >
> > > > > > HP BIOS Configuration driver purpose is to provide a driver supporting
> > > > > > the latest sysfs class firmware attributes framework allowing the user
> > > > > > to change BIOS settings and security solutions on HP Inc.’s commercial
> > > > > > notebooks.
> > > > > >
> > > > > > Many features of HP Commercial notebooks can be managed using Windows
> > > > > > Management Instrumentation (WMI). WMI is an implementation of Web-Based
> > > > > > Enterprise Management (WBEM) that provides a standards-based interface
> > > > > > for changing and monitoring system settings. HP BIOSCFG driver provides
> > > > > > a native Linux solution and the exposed features facilitates the
> > > > > > migration to Linux environments.
> > > > > >
> > > > > > The Linux security features to be provided in hp-bioscfg driver enables
> > > > > > managing the BIOS settings and security solutions via sysfs, a virtual
> > > > > > filesystem that can be used by user-mode applications. The new
> > > > > > documentation cover HP-specific firmware sysfs attributes such Secure
> > > > > > Platform Management and Sure Start. Each section provides security
> > > > > > feature description and identifies sysfs directories and files exposed
> > > > > > by the driver.
> > > > > >
> > > > > > Many HP Commercial notebooks include a feature called Secure Platform
> > > > > > Management (SPM), which replaces older password-based BIOS settings
> > > > > > management with public key cryptography. PC secure product management
> > > > > > begins when a target system is provisioned with cryptographic keys
> > > > > > that are used to ensure the integrity of communications between system
> > > > > > management utilities and the BIOS.
> > > > > >
> > > > > > HP Commercial notebooks have several BIOS settings that control its
> > > > > > behaviour and capabilities, many of which are related to security.
> > > > > > To prevent unauthorized changes to these settings, the system can
> > > > > > be configured to use a cryptographic signature-based authorization
> > > > > > string that the BIOS will use to verify authorization to modify the
> > > > > > setting.
> > > > > >
> > > > > > Linux Security components are under development and not published yet.
> > > > > > The only linux component is the driver (hp bioscfg) at this time.
> > > > > > Other published security components are under Windows.
> > > > > >
> > > > > > Signed-off-by: Jorge Lopez <jorge.lopez2@xxxxxx>
> > > > > >
> > > > > > ---
> > > > > > Based on the latest platform-drivers-x86.git/for-next
> > > > > > ---
>
> > > > > > + */
> > > > > > + if (count * LOG_ENTRY_SIZE > PAGE_SIZE)
> > > > > > + return -EIO;
> > > > > > +
> > > > > > + /*
> > > > > > + * We are guaranteed the buffer is 4KB so today all the event
> > > > > > + * logs will fit
> > > > > > + */
> > > > > > + for (i = 0; i < count; i++) {
> > > > > > + audit_log_buffer[0] = (i + 1);
> > > > > > +
> > > > > > + /*
> > > > > > + * read audit log entry at a time. 'buf' input value
> > > > > > + * provides the audit log entry to be read. On
> > > > > > + * input, Byte 0 = Audit Log entry number from
> > > > > > + * beginning (1..254)
> > > > > > + * Entry number 1 is the newest entry whereas the
> > > > > > + * highest entry number (number of entries) is the
> > > > > > + * oldest entry.
> > > > > > + */
> > > > > > + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG,
> > > > > > + HPWMI_SURESTART,
> > > > > > + audit_log_buffer, 1, 128);
> > > > > > +
> > > > > > + if (ret >= 0 && (LOG_ENTRY_SIZE * i) < PAGE_SIZE) {
> > > > >
> > > > > Can the second condition ever fail?
> > > > >
> > > > Only in the event BIOS data is corrupted.
> > >
> > > i runs from 0 to count - 1 and you prevented count * LOG_ENTRY_SIZE >
> > > PAGE_SIZE above. So what does the BIOS data have to do with that?
> >
> > BIOS guarantees the number of audit logs * LOG_ENTRY_SIZE will be less
> > than 4K (PAGE_SIZE)
> > Because Linux kernel trusts no one, we are checking that BIOS does not
> > report more events than it should.
>
> I know you're checking that.
>
> What I'm trying to say that even after that check, your own code does not
> trust that when i < count holds (as per the for loop termination
> condition), i * LOG_ENTRY_SIZE < count * LOG_ENTRY_SIZE.
>
> So what I'm trying to say is that this check:
> && (LOG_ENTRY_SIZE * i) < PAGE_SIZE
> ...is always true (and therefore unnecessary).
>
I partially agree. If BIOS returns a count size of 300 for the
current audit log entry size of 16 bytes, then this condition is
false.
(299 * 16 bytes (LOG_ENTRY_SIZE) ) is greater than 4K.
I am just trying to prevent conditions when BIOS could return invalid
audit_log_entry_count values.
If you still feel the code is unnecessary then I will proceed to
remove the check.
> > WMI expects the input buffer to include the current audit log number
> > (audit_log_buffer[0] = (i + 1);) which is i+1.
>
> I don't see how this is relevant to what I was asking.
>
Just adding information how the messages were retrieved.
Please ignore.
> > > > > > + memcpy(buf, audit_log_buffer, LOG_ENTRY_SIZE);
> > > > > > + buf += LOG_ENTRY_SIZE;
> > > > > > + } else {
> > > > > > + /*
> > > > > > + * Encountered a failure while reading
> > > > > > + * individual logs. Only a partial list of
> > > > > > + * audit log will be returned.
> > > > > > + */
> > > > > > + count = i + 1;
> > > > > > + break;
> > > > > > + }
> > > > >
> > > > > Reverse order, do error handling with break first.
> > > > Done!
> > > > >
> > > > > Why not return i * LOG_ENTRY_SIZE directly (or at the end), no need to
> > > > > tweak count?
> > > >
> > > > Done!
> > > > >
> > > > > > + }
> > > > > > +
> > > > > > + return count * LOG_ENTRY_SIZE;
> > > > > > +}
>
> --
> i.
