Hi Thomas,
On Tue, Apr 11, 2023 at 11:16 AM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> Hi Jorge!
>
>
> Apr 4, 2023 22:24:36 Jorge Lopez <jorgealtxwork@xxxxxxxxx>:
>
> > Hi Thomas,
> >
> > BTW, I decided to submit all files individually to facilitate the
> > review process. Only Makefile and Kconfig files will be provided as a
> > single patch.
> > I will be out of town until April 11 and will reply back upon my return.
>
> Sorry for the slow response.
>
> >
> > Please see my comments below.
> >
> >>
> >>> + HP specific types
> >>> + -----------------
> >>> + - ordered-list - a set of ordered list valid values
> >>> + - sure-start
> But what does it mean?
> For ordered-list there is a nice explanation.
>
The latest submission includes a short explanation
HP specific types
-----------------
- ordered-list - a set of ordered list valid values
- sure-start - report audit logs read from BIOS
> >>> + audit_log_entry_count:
> >>> + A read-only file that returns the number of existing audit log events available to be read.
> >>> + Values are separated using comma (``,``)
> >>> +
> >>> + [No of entries],[log entry size],[Max number of entries supported]
> >>
> >> Will log entry size always be 16? Or can it be bigger in the future when
> >> more bytes are used?
> >> This should be mentioned.
> >
> > Log entry size is always 16 bytes in size. The reason is to report a
> > maximum of 256 entries. Total 4096 bytes
>
> Does it make sense to expose the number 16 from sysfs if it never can change anyways?
The current audit log file is 16 bytes but future BIOS can expand the
audit log to up to 128 bytes.
Additional details are now included in the audit_log_entry_count
section. WMI reports only the total number of audit logs but not the
individual audit log size hence the reason the size is hardcoded to 16
bytes in the driver. A patch will be provided when the audit log size
changes or a new WMI is provided.
>
> Note you can also customize the filesize reported in sysfs to expose the maximum size to be used by userspace.
>
This is another area with additional details in the latest review.
"sure-start"-type specific properties:
audit_log_entries:
A read-only file that returns
the events in the log.
Values are separated using
semi-colon (``;``)
Audit log entry format
Byte 0-15: Requested Audit
Log entry (Each Audit log is 16 bytes)
Byte 16-127: Unused
audit_log_entry_count:
A read-only file that returns
the number of existing audit log events available to be read.
Values are separated using comma (``,``)
[No of entries],[log entry
size],[Max number of entries supported]
log entry size identifies
audit log size for the current BIOS version.
The current size is 16 bytes
but it can be to up to 128 bytes long
in future BIOS versions.
> >>
> >> Is audit_log_entry_count ever used without reading audit_log_entries
> >> right after?
> > Yes. The counter is necessary to determine how many logs are available
> > to be read.
>
> I think the cleaner interface would be to have users provide a buffer to read into and then they check the return value of read().
> Users can't trust the count value anyways as it is prone to TOCTOU races.
Audit logs buffer information includes an audit log number associated
with each log entry. The log number information is added by the
driver. The audit log is read as a single continuous buffer and the
application does not read individual audit logs while reading sysfs.
The application copies the whole buffer, extracts and identifies each
audit log from its local buffer.
Byte 0: Audit log number
Byte 1-16: Audit log information
>
> >
> >> If not the count file could be dropped.
> >>
> >>> +What: /sys/class/firmware-attributes/*/authentication/SPM/status
> >>> +Date: March 29
> >>> +KernelVersion: 5.18
> >>> +Contact: "Jorge Lopez" <jorge.lopez2@xxxxxx>
> >>> +Description: 'status' is a read-only file that returns ASCII text reporting
> >>> + the status information.
> >>> +
> >>> + State: Not Provisioned / Provisioned / Provisioning in progress
> >>> + Version: Major. Minor
> >>> + Feature Bit Mask: <16-bit unsigned number display in hex>
> >>
> >> How are these bits to be interpreted?
> > This information is provided by BIOS. It is one of those obscure
> > values from BIOS.
> >>
> >>> + SPM Counter: <16-bit unsigned number display in base 10>
> >>> + Signing Key Public Key Modulus (base64):
> >>> + KEK Public Key Modulus (base64):
> >>
> >> Is " (base64)" supposed to be part of the contents of the file?
> >
> > The information reported for Signing Key and KEK public key are
> > reported as base64 values. It applies only to the data and not to the
> > file contents.
>
> Put is the file format:
> KEK Public Key Modulus (base64): ...
> KEK Public Key Modulus: ...
>
> The docs indicate the former.
Also included in the last review is additional clarification on how
the Modules are reported.
What: /sys/class/firmware-attributes/*/authentication/SPM/status
Date: March 29
KernelVersion: 5.18
Contact: "Jorge Lopez" <jorge.lopez2@xxxxxx>
Description: 'status' is a read-only file that returns ASCII text reporting
the status information.
State: Not Provisioned / Provisioned / Provisioning
in progress
Version: Major. Minor
Feature Bit Mask: <16-bit unsigned number display in hex>
SPM Counter: <16-bit unsigned number display in base 10>
Signing Key Public Key Modulus (base64): <256 bytes
base64 in hex>
KEK Public Key Modulus (base64): <256 bytes base64 in hex>
>
> >>
> >>> +
> >>> +
> >>> +What: /sys/class/firmware-attributes/*/authentication/SPM/statusbin
> >>> +Date: March 29
> >>> +KernelVersion: 5.18
> >>> +Contact: "Jorge Lopez" <jorge.lopez2@xxxxxx>
> >>> +Description: 'statusbin' is a read-only file that returns identical status
> >>> + information reported by 'status' file in binary format.
> >>
> >> This documentation should contain enough information to understand the
> >> files contents.
> >>
> >>
> >> I understand that one WMI call will return all the fields that are part
> >> of the "status" and "statusbin" in one response.
> >>
> >> Are these WMI calls especially expensive or called especially
> >> frequently?
> >>
> >
> > Unfortunately the WMI to read the Status binary data is expensive
> > hence the reason of only calling once.
>
> Hm, I still dislike the interface, sorry.
No worries. The implementation is a legacy from the Windows driver
and security applications define it.
> What about caching the values in the driver and exposing them via different files?
The information changes as a group during the SPM configuration
process so caching would not help much. Version field is the only
field that can be cached.
If the information is split in difference files, there are conditions
when the user would read stale data in one field or another.
> >
> > Regards,
> > > Jorge
>
