| 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: <cf54c6f4-d177-4904-82ee-9d33566fb920@t-8ch.de>
Date: Sun, 23 Apr 2023 14:16:41 +0200
From: Thomas Weißschuh <thomas@...ch.de>
To: Jorge Lopez <jorgealtxwork@...il.com>
Cc: hdegoede@...hat.com, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 12/14] HP BIOSCFG driver - surestart-attributes
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
>
Powered by blists - more mailing lists