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 > > > --- > > > 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.