Message ID | 20230420165454.9517-13-jorge.lopez2@hp.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:6358:3046:b0:115:7a1d:dabb with SMTP id p6csp528697rwl; Thu, 20 Apr 2023 10:00:49 -0700 (PDT) X-Google-Smtp-Source: AKy350bBDTswmHgzLfk1gULimpkCPwbelqlWN/UrGKMJNZ+Yrg7mdT2e+RbtrKXRgFIbAAyZRD79 X-Received: by 2002:a05:6a21:1519:b0:f0:e69:748c with SMTP id nq25-20020a056a21151900b000f00e69748cmr2791766pzb.58.1682010049522; Thu, 20 Apr 2023 10:00:49 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682010049; cv=none; d=google.com; s=arc-20160816; b=hauvfFXNTxdmRveASx951lvu5PSQHcTlR1X2bBnTwTGEc+7SB29F4Nsve+9V3xjriV Z5rJMuSUDtFVWKnXIJ1pm2CoPugPlXRe2tcQgD1tQ1qmUYFQ/FeQenYqiAQvOYbZ7axz g+XZiB+AyWCNeHUYPD2gfJQMyio1spvCQG6VG/g7iMkwgDcWL+hRK+dc0uJpgRjwMGZQ ICzeVLih4ZdDD/gmIK+ITep9sHEl3c2b23Y6tP1Nw7CzoyoDJwuIvLlVbe+GAqmfrPyG lkvufv6e4JATtFfGW1Ot6gTLCkwVJJQR4ObpstOzM1iccvimNyz8Zm+J+XAfKaxBmFJR bRJw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:to:from :dkim-signature; bh=Obe2vSVBXXici55JN5j0R09I4VIG8/7jWFwQnKEIzYc=; b=RrHgyyKiEubsgVBBhN39HGgDpCbIcVdWz/d+xgO9uWSPeabbl7kBmywa5g0PbjlxGW oDR8DrfeFAZfRdo9MkAYcbzes/JM/UHGyWX9AbaRR8w6jNT1MIDCghzPsHlxrTEWrTo3 J3dI+eO0lEqp91JDDeXb7QvWKOJTmqzQU6bxUokrwPpcCr0i9kozR6Mnj8hqgbTgQAlh SEc3oWTH8Ixvy3kXN7LeIuVpifGK2MNX5uefhd/tNzsR1FWXAG12veU/oUbFBfDzZyxP LkoSGgim2T0t/s82uuXBoYah1sTp+nG2skquSZAYmJBc8PmSu2qTigrBDV5+Z98Z8dwb Kz+g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=n32strS6; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y2-20020aa793c2000000b0063b2655def0si2058841pff.47.2023.04.20.10.00.28; Thu, 20 Apr 2023 10:00:49 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=n32strS6; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230376AbjDTQz4 (ORCPT <rfc822;cjcooper78@gmail.com> + 99 others); Thu, 20 Apr 2023 12:55:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35158 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230334AbjDTQza (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 20 Apr 2023 12:55:30 -0400 Received: from mail-oa1-x32.google.com (mail-oa1-x32.google.com [IPv6:2001:4860:4864:20::32]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 14E04469E; Thu, 20 Apr 2023 09:55:11 -0700 (PDT) Received: by mail-oa1-x32.google.com with SMTP id 586e51a60fabf-187d9c205e9so710720fac.3; Thu, 20 Apr 2023 09:55:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1682009711; x=1684601711; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:from:to:cc:subject:date:message-id :reply-to; bh=Obe2vSVBXXici55JN5j0R09I4VIG8/7jWFwQnKEIzYc=; b=n32strS6mhAujrsaY1EeHfNtd2VqSI80/VYG4II3M82SRNe1Wkhpgd4nDvj+sjkfor +NAn5dXEnYZ4NncIRN8XNIMrhrBc+7BoVGFmMyhzQcxC85Zhw4+ZNcblIHSuvMrEGpFu rQR/LYGjt16Frd9+GInBz38LIdzl7IACGnGJKc9llUopKSlelisu++0u7hXtChNBFCHQ 3hBI4qovSs3qvb/HZ9OQOSWdq95oDUtnlc0vzxyyvR++iN50vaseDqnrPVv5qoxeIrb8 6yTEw33XtwOAQDTMKKgOO6NU2fUFoXEPduZjeTuY1m3KvlC0uA0T9oOZgp6VnfhEBFcV JusQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682009711; x=1684601711; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Obe2vSVBXXici55JN5j0R09I4VIG8/7jWFwQnKEIzYc=; b=Ipv9aiy+z47P+PJvueRGX2y6FX1gOGA8s8rWLOavHaD2VlENVRwyZaEmsEpABP2nG6 oMGLYr+0NiAr1G6ifHfqPuac8PAVXO5Y2cQ5q2ZHCMxVLrdPgHrEmpB1FK4j0rN7BS1D rDqpHuSxqD8qEie+sg7Yx4sGausqVeWuNYwSTD6N1PYqhpqWBj/S+1iQyDdnFI9Kbmkv bEyU/H4iqP1HXcj0lvaAcJIdybfA2PBcHUBV11za2onDmImz65sRV6vA/tRvuup2hCD0 ZbxQ22LMivHLfqx1iIoyn3VKBbnlL3MSM7AGzwqm+1k5p1UnUI6YBxG3DaRN0ziMkSpG ju8g== X-Gm-Message-State: AAQBX9eEWyyow6ujmJCf0cMj+ehQp/QVqZZB2t7aOF/+LCL21JMEWEK7 cyksbWvqNxgBXybzxtFlR4SKGXAqmcI= X-Received: by 2002:a05:6871:54c:b0:184:95:b822 with SMTP id t12-20020a056871054c00b001840095b822mr1593875oal.25.1682009711013; Thu, 20 Apr 2023 09:55:11 -0700 (PDT) Received: from grumpy-VECTOR.hsd1.tx.comcast.net ([2601:2c3:480:7390:d090:9746:e449:eb46]) by smtp.gmail.com with ESMTPSA id s129-20020a4a5187000000b005252e5b6604sm791913ooa.36.2023.04.20.09.55.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 Apr 2023 09:55:10 -0700 (PDT) From: Jorge Lopez <jorgealtxwork@gmail.com> X-Google-Original-From: Jorge Lopez <jorge.lopez2@hp.com> To: hdegoede@redhat.com, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org, thomas@t-8ch.de Subject: [PATCH v11 12/14] HP BIOSCFG driver - surestart-attributes Date: Thu, 20 Apr 2023 11:54:52 -0500 Message-Id: <20230420165454.9517-13-jorge.lopez2@hp.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230420165454.9517-1-jorge.lopez2@hp.com> References: <20230420165454.9517-1-jorge.lopez2@hp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1763715369606491802?= X-GMAIL-MSGID: =?utf-8?q?1763715369606491802?= |
Series |
HP BIOSCFG driver
|
|
Commit Message
Jorge Lopez
April 20, 2023, 4:54 p.m. UTC
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@hp.com>
---
Based on the latest platform-drivers-x86.git/for-next
---
.../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
Comments
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 > index 000000000000..72952758ffe3 > --- /dev/null > +++ b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c > @@ -0,0 +1,130 @@ > +// 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 <asm-generic/posix_types.h> Is the asm include needed? If yes, why not use linux/types.h? > + > +#define LOG_MAX_ENTRIES 254 A comment on how this values came to be would be good. > +#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)); > + > + if (ret < 0) > + return ret; > + > + return sysfs_emit(buf, "%d,%d,%d\n", count, LOG_ENTRY_SIZE, > + LOG_MAX_ENTRIES); > +} > + > +/* > + * 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; > + > + // Get the number of event logs > + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT, > + HPWMI_SURESTART, > + &count, 1, sizeof(count)); > + > + /* > + * The show() api will not work if the audit logs ever go > + * beyond 4KB > + */ > + if (count * LOG_ENTRY_SIZE > PAGE_SIZE) > + return -EFAULT; The error code seems not to match. Instead of not returning any data, why not show as many results as possible? > + > + if (ret < 0) > + return ret; > + > + /* > + * 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. > + *buf = (i + 1); Isn't this directly overwritten by the query below? > + 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. Can the audit-log not contain all-zero bytes? If it does this would need to be a bin_attribute. > + } > + > + return (count * LOG_ENTRY_SIZE); No need for braces. > +} > + > +static struct kobj_attribute sure_start_audit_log_entry_count = __ATTR_RO(audit_log_entry_count); > +static struct kobj_attribute sure_start_audit_log_entries = __ATTR_RO(audit_log_entries); > + > +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "sure-start\n"); > +} > +static struct kobj_attribute sure_start_type = __ATTR_RO(type); > + > +static ssize_t display_name_language_code_show(struct kobject *kobj, > + struct kobj_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "%s\n", LANG_CODE_STR); > +} > + > +static struct kobj_attribute sure_start_display_langcode = > + __ATTR_RO(display_name_language_code); > + > + > +static ssize_t display_name_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + return sysfs_emit(buf, "%s\n", SURE_START_DESC); > +} > +static struct kobj_attribute sure_start_display_name = __ATTR_RO(display_name); > + > +static struct attribute *sure_start_attrs[] = { > + &sure_start_display_name.attr, > + &sure_start_display_langcode.attr, > + &sure_start_audit_log_entry_count.attr, > + &sure_start_audit_log_entries.attr, > + &sure_start_type.attr, > + NULL > +}; > + > +static const struct attribute_group sure_start_attr_group = { > + .attrs = sure_start_attrs, > +}; > + > +void exit_sure_start_attributes(void) > +{ > + sysfs_remove_group(bioscfg_drv.sure_start_attr_kobj, > + &sure_start_attr_group); > +} > + > +int populate_sure_start_data(struct kobject *attr_name_kobj) > +{ > + bioscfg_drv.sure_start_attr_kobj = attr_name_kobj; > + return sysfs_create_group(attr_name_kobj, &sure_start_attr_group); > +} > -- > 2.34.1 >
On Sun, Apr 23, 2023 at 7:16 AM Thomas Weißschuh <thomas@t-8ch.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 > > index 000000000000..72952758ffe3 > > --- /dev/null > > +++ b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c > > @@ -0,0 +1,130 @@ > > +// 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 <asm-generic/posix_types.h> > > Is the asm include needed? > If yes, why not use linux/types.h? > Will change in Version 12 > > + > > +#define LOG_MAX_ENTRIES 254 > > A comment on how this values came to be would be good. > Done! > > +#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)); > > + > > + if (ret < 0) > > + return ret; > > + > > + return sysfs_emit(buf, "%d,%d,%d\n", count, LOG_ENTRY_SIZE, > > + LOG_MAX_ENTRIES); > > +} > > + > > +/* > > + * 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; > > + > > + // Get the number of event logs > > + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT, > > + HPWMI_SURESTART, > > + &count, 1, sizeof(count)); > > + > > + /* > > + * The show() api will not work if the audit logs ever go > > + * beyond 4KB > > + */ > > + if (count * LOG_ENTRY_SIZE > PAGE_SIZE) > > + return -EFAULT; > > The error code seems not to match. > Changing error to -EINVAL > 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 > > + > > + if (ret < 0) > > + return ret; > > + > > + /* > > + * 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. > > > + 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. The audit log file cannot contain all zero bytes. > > > + } > > + > > + return (count * LOG_ENTRY_SIZE); > > No need for braces. Done! > <snip>
On 2023-04-27 17:17:57-0500, Jorge Lopez wrote: > On Sun, Apr 23, 2023 at 7:16 AM Thomas Weißschuh <thomas@t-8ch.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 > > > index 000000000000..72952758ffe3 > > > --- /dev/null > > > +++ b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c > > > @@ -0,0 +1,130 @@ <snip> > > > + > > > +/* > > > + * 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; > > > + > > > + // Get the number of event logs > > > + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT, > > > + HPWMI_SURESTART, > > > + &count, 1, sizeof(count)); > > > + > > > + /* > > > + * The show() api will not work if the audit logs ever go > > > + * beyond 4KB > > > + */ > > > + if (count * LOG_ENTRY_SIZE > PAGE_SIZE) > > > + return -EFAULT; > > > > The error code seems not to match. > > > > Changing error to -EINVAL -EIO seems better. The problem is not due to some value a user passed but an unhandled from the hardware. > > > 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. > > > + > > > + if (ret < 0) > > > + return ret; And this should first validate ret and then count. > > > + > > > + /* > > > + * 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 needs a comment. > > > > > + 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. > The audit log file cannot contain all zero bytes. I doublechecked this and zero bytes seem to also be fine in normal text attributes. > > > + return (count * LOG_ENTRY_SIZE); If one of the calls to hp_wmi_perform_query() fails this return value is wrong, it does not reflect the amount of actually written data.
On Fri, Apr 28, 2023 at 1:03 AM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.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 > > > > index 000000000000..72952758ffe3 > > > > --- /dev/null > > > > +++ b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c > > > > @@ -0,0 +1,130 @@ > > <snip> > > > > > + > > > > +/* > > > > + * 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; > > > > + > > > > + // Get the number of event logs > > > > + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT, > > > > + HPWMI_SURESTART, > > > > + &count, 1, sizeof(count)); > > > > + > > > > + /* > > > > + * The show() api will not work if the audit logs ever go > > > > + * beyond 4KB > > > > + */ > > > > + if (count * LOG_ENTRY_SIZE > PAGE_SIZE) > > > > + return -EFAULT; > > > > > > The error code seems not to match. > > > > > > > Changing error to -EINVAL > > -EIO seems better. Done! > > The problem is not due to some value a user passed but an unhandled from > the hardware. > > > > > > 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. > > > > > + > > > > + 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. > > The audit log file cannot contain all zero bytes. > > I doublechecked this and zero bytes seem to also be fine in normal text > attributes. > > > > > + return (count * LOG_ENTRY_SIZE); > > If one of the calls to hp_wmi_perform_query() fails this return value is wrong, > it does not reflect the amount of actually written data. Version 12 of the code takes such a condition in consideration and recalculates the value of 'count' to match the one number of valid audit logs read.
On 2023-04-28 09:58:01-0500, Jorge Lopez wrote: > On Fri, Apr 28, 2023 at 1:03 AM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.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. Storing it in a 128 byte stackvariable would also sidestep the issue.
On Fri, Apr 28, 2023 at 10:21 AM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.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@t-8ch.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.
On 2023-04-28 10:40:59-0500, Jorge Lopez wrote: > On Fri, Apr 28, 2023 at 10:21 AM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.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@t-8ch.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)
On Fri, Apr 28, 2023 at 11:06 AM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.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@t-8ch.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@t-8ch.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!
On Fri, Apr 28, 2023 at 11:06 AM Thomas Weißschuh <thomas@t-8ch.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@t-8ch.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@t-8ch.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@t-8ch.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!
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..72952758ffe3 --- /dev/null +++ b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c @@ -0,0 +1,130 @@ +// 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 <asm-generic/posix_types.h> + +#define LOG_MAX_ENTRIES 254 +#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)); + + if (ret < 0) + return ret; + + return sysfs_emit(buf, "%d,%d,%d\n", count, LOG_ENTRY_SIZE, + LOG_MAX_ENTRIES); +} + +/* + * 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; + + // Get the number of event logs + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT, + HPWMI_SURESTART, + &count, 1, sizeof(count)); + + /* + * The show() api will not work if the audit logs ever go + * beyond 4KB + */ + if (count * LOG_ENTRY_SIZE > PAGE_SIZE) + return -EFAULT; + + if (ret < 0) + return ret; + + /* + * We are guaranteed the buffer is 4KB so today all the event + * logs will fit + */ + + for (i = 0; ((i < count) & (ret >= 0)); i++) { + *buf = (i + 1); + ret = hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG, + HPWMI_SURESTART, + buf, 1, 128); + if (ret >= 0) + buf += LOG_ENTRY_SIZE; + } + + return (count * LOG_ENTRY_SIZE); +} + +static struct kobj_attribute sure_start_audit_log_entry_count = __ATTR_RO(audit_log_entry_count); +static struct kobj_attribute sure_start_audit_log_entries = __ATTR_RO(audit_log_entries); + +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "sure-start\n"); +} +static struct kobj_attribute sure_start_type = __ATTR_RO(type); + +static ssize_t display_name_language_code_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%s\n", LANG_CODE_STR); +} + +static struct kobj_attribute sure_start_display_langcode = + __ATTR_RO(display_name_language_code); + + +static ssize_t display_name_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + return sysfs_emit(buf, "%s\n", SURE_START_DESC); +} +static struct kobj_attribute sure_start_display_name = __ATTR_RO(display_name); + +static struct attribute *sure_start_attrs[] = { + &sure_start_display_name.attr, + &sure_start_display_langcode.attr, + &sure_start_audit_log_entry_count.attr, + &sure_start_audit_log_entries.attr, + &sure_start_type.attr, + NULL +}; + +static const struct attribute_group sure_start_attr_group = { + .attrs = sure_start_attrs, +}; + +void exit_sure_start_attributes(void) +{ + sysfs_remove_group(bioscfg_drv.sure_start_attr_kobj, + &sure_start_attr_group); +} + +int populate_sure_start_data(struct kobject *attr_name_kobj) +{ + bioscfg_drv.sure_start_attr_kobj = attr_name_kobj; + return sysfs_create_group(attr_name_kobj, &sure_start_attr_group); +}