| 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
| ||
|
Message-ID: <79db2a99-5cd7-19c0-212d-9e28869a6a18@linux.intel.com>
Date: Thu, 11 May 2023 12:32:51 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Jorge Lopez <jorgealtxwork@...il.com>
cc: hdegoede@...hat.com, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, thomas@...ch.de
Subject: Re: [PATCH v12 11/13] HP BIOSCFG driver - surestart-attributes
On Wed, 10 May 2023, Jorge Lopez wrote:
> On Tue, May 9, 2023 at 8:57 AM Ilpo Järvinen
> <ilpo.jarvinen@...ux.intel.com> 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@...com>
> > >
> > > ---
> > > Based on the latest platform-drivers-x86.git/for-next
> > > ---
> > > 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..b627c324f6a6
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c
> > > @@ -0,0 +1,133 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Functions corresponding to sure start object type attributes under
> > > + * BIOS for use with hp-bioscfg driver
> > > + *
> > > + * Copyright (c) 2022 HP Development Company, L.P.
> > > + */
> > > +
> > > +#include "bioscfg.h"
> > > +#include <linux/types.h>
> > > +
> > > +/* Maximum number of log entries supported when log entry size is 16
> > > + * bytes. This value is calculated by dividing 4096 (page size) by
> > > + * log entry size.
> > > + */
> > > +#define LOG_MAX_ENTRIES 254
> > > +
> > > +/*
> > > + * Current Log entry size. This value size will change in the
> > > + * future. The driver reads a total of 128 bytes for each log entry
> > > + * provided by BIOS but only the first 16 bytes are used/read.
> > > + */
> > > +#define LOG_ENTRY_SIZE 16
> > > +
> > > +/*
> > > + * audit_log_entry_count_show - Reports the number of
> > > + * existing audit log entries available
> > > + * to be read
> > > + */
> > > +static ssize_t audit_log_entry_count_show(struct kobject *kobj,
> > > + struct kobj_attribute *attr, char *buf)
> > > +{
> > > + int ret;
> > > + u32 count = 0;
> > > +
> > > + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT,
> > > + HPWMI_SURESTART,
> > > + &count, 1, sizeof(count));
> > > +
> >
> > Extra newline.
> Done!
> >
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return sysfs_emit(buf, "%d,%d,%d\n", count, LOG_ENTRY_SIZE,
> > > + LOG_MAX_ENTRIES);
> >
> > Why 3 values instead of 1?
> This version of BIOS only returns the number of audit log events available.
> The other two values are the current log entry size which today is
> hardcoded. This will change in future when BIOS returns the log entry
> size.
And you cannot provide the others in separate sysfs files?
> > > +}
> > > +
> > > +/*
> > > + * 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;
> > > + u8 audit_log_buffer[128];
> > > +
> > > + // Get the number of event logs
> > > + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT,
> > > + HPWMI_SURESTART,
> > > + &count, 1, sizeof(count));
> > > +
> >
> > Extra newline.
> Done!
> >
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /*
> > > + * The show() api will not work if the audit logs ever go
> > > + * beyond 4KB
> >
> > Extra space.
> Done!
> >
> > > + */
> > > + 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);
> >
> > Extra parenthesis.
> Done!
> >
> > > +
> > > + /*
> > > + * read audit log entry at a time. 'buf' input value
> > > + * provides the audit log entry to be read. On
> >
> > Extra spaces.
> Done!
> >
> > > + * 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 the BIOS data has to do with that?
> > > + 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.
Powered by blists - more mailing lists