[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH] dmioem: Decode HPE OEM Record 224
|
From: |
Jerry Hoemann |
|
Subject: |
Re: [dmidecode] [PATCH] dmioem: Decode HPE OEM Record 224 |
|
Date: |
Mon, 27 Jun 2022 14:28:44 -0600 |
|
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Thu, Jun 23, 2022 at 01:20:54PM +0200, Jean Delvare wrote:
> Hi Jerry,
>
> On Tue, 21 Jun 2022 15:59:07 -0600, Jerry Hoemann wrote:
> > Decode HPE OEM Record 224: Trusted Module (TPM or TCM) Status (Type 224)
> >
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> > dmioem.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 119 insertions(+)
> >
> > diff --git a/dmioem.c b/dmioem.c
> > index c26fff9..982e94e 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -319,6 +319,95 @@ static void dmi_hp_238_loc(const char *fname, unsigned
> > int code)
> > pr_attr(fname, "%s", str);
> > }
> >
> > +static int dmi_hp_224_status(u32 code)
>
> I'd prefer to have these dmi_hp_224_* helper functions after the
> dmi_hp_238_* helper function rather than in the middle of them.
Sorry, merge error when updating my branch. The 224 helper functions
were supposed to be before the 238 functions as 224 < 238. Will fix.
>
> > +{
> > + static const char * const present[] = {
> > + "Not Present", /* 0x00 */
> > + "Present/Enabled",
> > + "Present/Disabled",
> > + "Reserved" /* 0x03 */
> > + };
> > +
> > + pr_attr("Status", "%s", present[code & 0x03]);
> > + if ((code & 0x03) == 0x00)
> > + return 0;
> > + pr_attr("Option ROM Measuring", "%s", (code & (1 << 2)) ? "Yes" : "No");
>
> For my own enlightenment, what does "Option ROM Measuring" mean?
In this context TPM measurements are essentially checksums of a
thing. In this case it's the Option ROM. So this tells if the system in
its current configuration is measuring the option rom or not.
(A message is actually printed during POST for this.)
>
> > + pr_attr("Visible/Hidden", "%s", (code & (1 << 3)) ? "Hidden" :
> > "Visible");
>
> For consistency I'd go with "Hidden", "Yes", "No".
done.
>
> > + return 1;
> > +}
> > +
> > +static void dmi_hp_224_ex_status(u32 status, u32 code)
> > +{
> > + const char *str = "Reserved";
> > + static const char * const disable_reason[] = {
> > + "Not Specified", /* 0x00 */
> > + "User Disabled",
> > + "Error Condition",
> > + "Reserved" /* 0x03 */
> > + };
> > + static const char * const error_condition[] = {
> > + "Not Specified", /* 0x00 */
> > + "Self-Test", /* 0x01 */
> > + };
> > + if ((status & 0x03) == (1 << 1))
>
> status & 0x03 is not a bit field, it's a 2-bit value, so comparing it
> with 1 << 1 doesn't really make sense. You should check for 0x02
> instead.
done.
>
> > + pr_attr("Disable Reason", "%s", disable_reason[code & 0x03]);
> > + if ((code & 0x03) == (1 << 1)) {
>
> Same here.
done
>
> > + if (((code >> 2) & 0x0f) < ARRAY_SIZE(error_condition))
> > + str = error_condition[(code >> 2) & 0x0f];
>
> Storing (code >> 2) & 0x0f in a local variable would make the code
> clearer IMHO.
done
>
> > + pr_attr("Error Condition", "%s", str);
> > + }
> > +}
> > +
> > +static void dmi_hp_224_module_type(u32 code)
>
> The size of the "code" parameter is not consistent with the value
> passed to the function.
fixed throughout patch.
>
> > +{
> > + const char *str = "Reserved";
> > + static const char * const type[] = {
> > + "Not Specified", /* 0x00 */
> > + "TPM 1.2",
> > + "TPM 2.0",
> > + "Intel PTT fTPM" /* 0x03 */
> > + };
> > + if ((code & 0x0f) < ARRAY_SIZE(type))
> > + str = type[code & 0x0f];
> > + pr_attr("Type", "%s", str);
> > + pr_attr("Standard Algorithm Supported", "%s", (((code >> 4) & 0x0f) &
> > (1 << 0)) ? "Yes" : "No");
> > + pr_attr("Chinese Algorithm Supported", "%s", (((code >> 4) & 0x0f) &
> > (1 << 1)) ? "Yes" : "No");
>
> Duplicate space after comma (no, we don't do opportunistic code
> alignment in dmidecode).
:( :(
>
> There's some repeated arithmetic here that could be avoided. Either
> access the bits directly:
>
> pr_attr("Standard Algorithm Supported", "%s", code & (1 << 4) ? "Yes" :
> "No");
How about:
pr_attr("Standard Algorithm Supported", "%s", (code & (1 << 4)) ? "Yes"
: "No");
>
> Or ues a local variable to store the extracted field:
>
> u8 flags = (code >> 4) & 0x0f;
>
> (...)
> pr_attr("Standard Algorithm Supported", "%s", flags & (1 << 0) ? "Yes"
> : "No");
>
> Both make the code more readable IMHO. I'd go for the first option for
> simplicity but I'm fine either way. If you go for the second option,
> note that masking is technically not needed if code is a u8 as I think
> it should.
>
> > +}
> > +
> > +static void dmi_hp_224_module_attr(u32 code)
>
> Parameter size mismatch.
fixed.
>
> > +{
> > + static const char * const phys_attr[] = {
> > + "Not Specified", /* 0x00 */
> > + "Pluggable and Optional",
> > + "Plubaggable but Stanard",
>
> I don't know these words :-D
fixed.
>
> I also don't really understand how "Standard" opposes itself to
> "Pluggable" thus can't really make sense of the "but" between them.
Sans typo, this is directly from the spec.
The physical nature of the TPM on proliant systems has been evolving and
can vary based upon system. The vision (as I understand it) roughly speaking
is:
Phase one, the TPM was a could be added my customers, hence plugable.
For example:
https://techlibrary.hpe.com/docs/iss/DL380_Gen10/setup_install/GUID-4D7C8B58-F938-4A6B-B0E2-D5EFDFCCBF07.html#GUID-4D7C8B58-F938-4A6B-B0E2-D5EFDFCCBF07
The coffin that one puts over the TPM adds tamper resistence to the
device.
Phase two, the device was essentially the same, but was to be
a standard feature (i.e. installed by the factory.)
Hence, "Plugable but Standard"
The third phase would be a TPM was soldered down on the motherboard.
Having said this, I'm not sure to what extent this vision was realized
or if FW is reporting it correctly. All systems that I have access to
that have TPM show as "Plugable and Optional."
>
> > + "Soldered Down on System Board" /* 0x03 */
> > + };
> > + static const char * const fips_attr[] = {
> > + "Not Specified", /* 0x00 */
> > + "Not FIPS Certified",
> > + "FIPS Certified",
> > + "Reserved" /* 0x03 */
> > + };
> > + pr_attr("Attributes", "%s", phys_attr[code & 0x3]);
>
> "Attributes" seems a little vague as a description of this field. Can
> we use something more descriptive, like "Physical Format" or "Form
> Factor" maybe?
Field is documented as "Trusted Module Attributes." I'll switch to
that.
>
> > + pr_attr("FIPS Certification", "%s", fips_attr[((code >> 2) & 0x03)]);
> > +}
> > +
> > +static void dmi_hp_224_chipid(u32 code)
>
> Parameter size mismatch again.
word => u16.
done.
>
> > +{
> > + const char *str = "Reserved";
> > + static const char * const chipid[] = {
> > + "None", /* 0x00 */
> > + "STMicroGen10 TPM",
> > + "Intel firmware TPM (PTT)",
> > + "Nationz TPM",
> > + "STMicroGen10 Plus TPM",
> > + "STMicroGen11 TPM", /* 0x05 */
> > + };
> > + if ((code & 0xff) < ARRAY_SIZE(chipid))
> > + str = chipid[code & 0xff];
> > + pr_attr("Chip Identifier", "%s", str);
> > +}
> > +
> > static void dmi_hp_238_flags(const char *fname, unsigned int code)
> > {
> > const char *str = "Reserved";
> > @@ -599,6 +688,36 @@ static int dmi_decode_hp(const struct dmi_header *h)
> > pr_subattr("UEFI", "%s", feat & 0x1400 ? "Yes" : "No");
> > break;
> >
> > + case 224:
> > + /*
> > + * Vendor Specific: Trusted Module (TPM or TCM) Status
> > + *
> > + * Offset | Name | Width | Description
> > + * -------------------------------------
> > + * 0x00 | Type | BYTE | 0xE0, Trusted Module (TPM
> > or TCM) Status
>
> "Type" should be left-aligned for consistency (although I see type 233
> has is wrong too).
done
Will address record 233 as part of another non-functional patch
that I am planning.
>
> > + * 0x01 | Length | BYTE | Length of structure
> > + * 0x02 | Handle | WORD | Unique handle
> > + * 0x04 | Status | BYTE | Status Flag Byte
> > + * 0x05 | Ex Stat| BYTE | TPM Extended Status
> > + * 0x06 | Type | BYTE | Trusted Module Type
> > + * 0x07 | Attrib | BYTE | Trusted Module Attributes
> > + * 0x08 | Handle | WORD | Handle to map to Type 216
> > + * 0x0A | Chip ID| WORD | Chip Identifier Values
> > + */
> > + pr_handle_name("%s Trusted Module (TPM or TCM) Status",
> > company);
> > + if (h->length < 0x05) break;
> > + if (!dmi_hp_224_status(data[0x04]))
> > + break;
> > + if (h->length < 0x0a) break;
> > + dmi_hp_224_ex_status(data[0x04], data[0x05]);
> > + dmi_hp_224_module_type(data[0x06]);
> > + dmi_hp_224_module_attr(data[0x07]);
> > + if (!(opt.flags & FLAG_QUIET))
> > + pr_attr("Associated Handle", "0x%04X",
> > WORD(data + 0x8));
> > + if (h->length < 0x0c) break;
> > + dmi_hp_224_chipid(WORD(data + 0x0a));
> > + break;
> > +
> > case 233:
> > /*
> > * Vendor Specific: HPE ProLiant NIC MAC Information
>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------