Re: [PATCH v1 4/6] Sure Start Security Feature

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 5, 2022 at 12:09 PM Limonciello, Mario
<Mario.Limonciello@xxxxxxx> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Jorge Lopez <jorgealtxwork@xxxxxxxxx>
> > Sent: Tuesday, April 5, 2022 11:56
> > To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> > Cc: platform-driver-x86@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v1 4/6] Sure Start Security Feature
> >
> > hi Mario,
> >
> > On Mon, Apr 4, 2022 at 5:05 PM Limonciello, Mario
> > <Mario.Limonciello@xxxxxxx> wrote:
> > >
> > > [Public]
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jorge Lopez <jorgealtxwork@xxxxxxxxx>
> > > > Sent: Monday, April 4, 2022 15:36
> > > > To: platform-driver-x86@xxxxxxxxxxxxxxx
> > > > Subject: [PATCH v1 4/6] Sure Start Security Feature
> > > >
> > > > Sure Start provides advanced firmware protection and resiliency by
> > > > identifying and repairing unauthorized BIOS changes.  It maintains an
> > > > audit log of these events and other important system configuration
> > > > changes.  The following sysfs entries can be used to read the contents
> > > > of the audit log.
> > > >
> > > >       /sys/devices/platform/hp-wmi/sure_start/audit_log_entry_count
> > > >       /sys/devices/platform/hp-wmi/sure_start/audit_log_entries
> > > >
> > > > 'audit_log_entry_count' is a read-only file that returns the number of
> > > > existing audit log events available to be read
> > > >
> > > > 'audit_log_entries' is a read-only file that returns the events in the
> > > > log
> > > >
> > > > This feature requires "Update hp_wmi_group to simplify feature
> > > > addition" patch.
> > > >
> > > > All changes were validated on a HP ZBook Workstation,
> > > > HP EliteBook x360, and HP EliteBook 850 G8 notebooks.
> > > >
> > > > Signed-off-by: Jorge Lopez <jorge.lopez2@xxxxxx>
> > > >
> > > > ---
> > > > Based on the latest platform-drivers-x86.git/for-next
> > > > ---
> > > >  drivers/platform/x86/hp-wmi.c | 108
> > > > ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 108 insertions(+)
> > > >
> > > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-
> > wmi.c
> > > > index 139dc079c1fa..918e3eaf1b67 100644
> > > > --- a/drivers/platform/x86/hp-wmi.c
> > > > +++ b/drivers/platform/x86/hp-wmi.c
> > > > @@ -126,6 +126,11 @@ enum hp_wmi_spm_commandtype {
> > > >       HPWMI_SECUREPLATFORM_SET_SK     = 0x12,
> > > >  };
> > > >
> > > > +enum hp_wmi_surestart_commandtype {
> > > > +     HPWMI_SURESTART_GET_LOG_COUNT   = 0x01,
> > > > +     HPWMI_SURESTART_GET_LOG = 0x02,
> > > > +};
> > > > +
> > > >  enum hp_wmi_gm_commandtype {
> > > >       HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> > > >       HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> > > > @@ -138,6 +143,7 @@ enum hp_wmi_command {
> > > >       HPWMI_READ      = 0x01,
> > > >       HPWMI_WRITE     = 0x02,
> > > >       HPWMI_ODM       = 0x03,
> > > > +     HPWMI_SURESTART = 0x20006,
> > > >       HPWMI_GM        = 0x20008,
> > > >       HPWMI_SECUREPLATFORM = 0x20010,
> > > >  };
> > > > @@ -851,6 +857,7 @@ static ssize_t spm_kek_store(struct kobject *kobj,
> > > >  {
> > > >       int ret =
> > > > hp_wmi_perform_query(HPWMI_SECUREPLATFORM_SET_KEK,
> > > >                                      HPWMI_SECUREPLATFORM, (void *)buf,
> > > > count, 0);
> > > > +
> > > >       return ret ? -EINVAL : count;
> > > >  }
> > > >
> > > > @@ -918,6 +925,106 @@ static const struct attribute_group spm_group =
> > {
> > > >       .attrs = spm_attrs,
> > > >  };
> > > >
> > > > +/* Sure Start functions */
> > > > +
> > > > +#define LOG_MAX_ENTRIES      254
> > > > +#define LOG_ENTRY_SIZE       16
> > > > +
> > > > +/*
> > > > + * sure_start_audit_log_entry_count_show - Reports the number of
> > > > + *                           existing audit log entries available
> > > > + *                           to be read
> > > > + *
> > > > + * @kobj:  Pointer to a kernel object of things that show up as directory
> > > > + *      in the sysfs filesystem.
> > > > + * @attr:  Pointer to list of attributes for the operation
> > > > + * @buf:   Pointer to buffer
> > > > + *
> > > > + * Returns number of existing audit log entries available to be read,
> > > > + *         audit log entry size, and maximum number of entries
> > > > + *         supported. Otherwise, an HP WMI query specific error code
> > > > + *         (which is negative)
> > > > + *
> > > > + *         [No of entries],[log entry size],[Max number of entries
> > supported]
> > > > + */
> > > > +static ssize_t sure_start_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, 0, sizeof(count));
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > > +
> > > > +     return snprintf(buf, PAGE_SIZE, "%d,%d,%d\n", count,
> > > > LOG_ENTRY_SIZE, LOG_MAX_ENTRIES);
> > > > +}
> > > > +
> > > > +/*
> > > > + * sure_start_audit_log_entries_show() - Return all entries found in log
> > file
> > > > + *
> > > > + * @kobj:  Pointer to a kernel object of things that show up as
> > > > + *      directory in the sysfs filesystem.
> > > > + * @attr:  Pointer to list of attributes for the operation
> > > > + * @buf:   Pointer to buffer
> > > > + *
> > > > + * Returns number of bytes needed to read all audit logs entries to be
> > read.
> > > > + *         Otherwise, an HP WMI query specific error code (which is
> > negative)
> > > > + *      -EFAULT if the audit logs size exceeds 4KB
> > > > + *
> > > > + */
> > > > +static ssize_t sure_start_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, 4);
> > > > +
> > > > +     /*
> > > > +      * The show() api will not work if the audit logs ever go
> > > > +      *  beyond 4KB
> > > > +      */
> > > > +     if (count * LOG_ENTRY_SIZE > PAGE_SIZE)
> > > > +             return -EFAULT;
> > >
> > > This is an interface that will be there forever, what are realistic numbers
> > and sizes after a long time?
> > > I have an AMD HP laptop that is only been used a few months and checked
> > the size like this:
> > > dd if=audit_log_entries of=/tmp/entries
> > >
> > The audit log will grow linearly and it is defined by BIOS.  The total
> > number of bytes ( audit log size times the max number of audit logs
> > supported by BIOS) is 4K.
> > The audit log size reports three values. Number of event logs, audit
> > log max size (16), and max of audit logs supported by BIOS (254).
> > (16*254 = 4K).
> > In your case, the total number of bytes for all audit logs available
> > is 224 bytes.  You can conclude, your system has 14 audit logs
> > available to be read  (224/16 = 14)
>
>  But so if this is growing linearly what happens if BIOS ends up with
>  more than 4k audit logs?  Can that happen?

BIOS limits the max total audit log size to 4k.  BIOS will discard the
oldest and maintain the max 4k size.

>
> If that happens  I can no longer access them from userspace right?

If it happens, the driver will return an error and  no audit logs will
be accessible from userspace..

>
> >
> > > If so, then maybe this should be designed as a different interface.
> > >
> > > Also, the log is readable by anybody.  Should this be root only?
> >
> > No.  audit logs will be available to everyone.  The interpretation of
> > each audit event is done at the application level.
>
> Got it - thanks
>
> >
> > >
> > > > +
> > > > +     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);
> > > > +}
> > > > +
> > > > +HPWMI_ATTR_RO(sure_start, audit_log_entry_count);
> > > > +HPWMI_ATTR_RO(sure_start, audit_log_entries);
> > > > +
> > > > +static struct attribute *sure_start_attrs[] = {
> > > > +     &sure_start_audit_log_entry_count.attr,
> > > > +     &sure_start_audit_log_entries.attr,
> > > > +     NULL,
> > > > +};
> > > > +
> > > > +static const struct attribute_group sure_start_group = {
> > > > +     .name = "sure_start",
> > > > +     .attrs = sure_start_attrs,
> > > > +};
> > > > +
> > > >  static DEVICE_ATTR_RO(display);
> > > >  static DEVICE_ATTR_RO(hddtemp);
> > > >  static DEVICE_ATTR_RW(als);
> > > > @@ -942,6 +1049,7 @@ static const struct attribute_group
> > hp_wmi_group =
> > > > {
> > > >  static const struct attribute_group *hp_wmi_groups[] = {
> > > >       &hp_wmi_group,
> > > >       &spm_group,
> > > > +     &sure_start_group,
> > > >       NULL,
> > > >  };
> > > >
> > > > --
> > > > 2.25.1



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux