dmicheck: fix handling of type 16 capacities > 2 TiB
diff mbox series

Message ID 20181003002351.24394-1-elliott@hpe.com
State Superseded
Headers show
Series
  • dmicheck: fix handling of type 16 capacities > 2 TiB
Related show

Commit Message

Elliott, Robert (Servers) Oct. 3, 2018, 12:23 a.m. UTC
In the SMBIOS type 16 (Physical Memory Array) record, a Maximum Capacity
field of 0x80000000 means that the capacity is reported in the
Extended Maximum Capacity field.  However, that value triggers this
error:

FAILED [HIGH] DMIValueOutOfRange: Test 2, Out of range value 0x80000000 (range
allowed 0x0000..0x7fffffff) while accessing entry 'Physical Memory Array (Type
16)' @ 0x8a02f1fd, field 'Maximum Capacity', offset 0x07

check: Out of range value 0x80000000 (range allowed 0x0000..0x7fffffff) while accessing entry 'Physical Memory Array (Type 16)' @ 0x8a02f1fd, field 'Maximum Capacity',
 offset 0x07

Modify the Maximum Capacity range check to exclude that value.

Signed-off-by: Robert Elliott <elliott@hpe.com>
---
 src/dmi/dmicheck/dmicheck.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Alex Hung Oct. 3, 2018, 12:33 a.m. UTC | #1
Hi Robert,

Thanks for the patch, and this seems to be similar to
http://patchwork.ozlabs.org/patch/974876/.

Do you have comments on the other one, ex. yours is more complete?
On Wed, Oct 3, 2018 at 8:21 AM Robert Elliott <elliott@hpe.com> wrote:
>
> In the SMBIOS type 16 (Physical Memory Array) record, a Maximum Capacity
> field of 0x80000000 means that the capacity is reported in the
> Extended Maximum Capacity field.  However, that value triggers this
> error:
>
> FAILED [HIGH] DMIValueOutOfRange: Test 2, Out of range value 0x80000000 (range
> allowed 0x0000..0x7fffffff) while accessing entry 'Physical Memory Array (Type
> 16)' @ 0x8a02f1fd, field 'Maximum Capacity', offset 0x07
>
> check: Out of range value 0x80000000 (range allowed 0x0000..0x7fffffff) while accessing entry 'Physical Memory Array (Type 16)' @ 0x8a02f1fd, field 'Maximum Capacity',
>  offset 0x07
>
> Modify the Maximum Capacity range check to exclude that value.
>
> Signed-off-by: Robert Elliott <elliott@hpe.com>
> ---
>  src/dmi/dmicheck/dmicheck.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c
> index d0096bd4..3733fd98 100644
> --- a/src/dmi/dmicheck/dmicheck.c
> +++ b/src/dmi/dmicheck/dmicheck.c
> @@ -1512,7 +1512,8 @@ static void dmicheck_entry(fwts_framework *fw,
>                                         data[0x4], table, addr, "Location", 0x4);
>                         dmi_min_max_uint8_check(fw, table, addr, "Use", hdr, 0x5, 0x1, 0x7);
>                         dmi_min_max_uint8_check(fw, table, addr, "Error Corrrection Type", hdr, 0x6, 0x1, 0x7);
> -                       dmi_min_max_uint32_check(fw, table, addr, "Maximum Capacity", hdr, 0x7, 0, 0x80000000 - 1);
> +                       if (GET_UINT32(data + 0x7) != 0x80000000)
> +                               dmi_min_max_uint32_check(fw, table, addr, "Maximum Capacity", hdr, 0x7, 0, 0x80000000 - 1);
>                         if (hdr->length < 0x17)
>                                 break;
>                         if (GET_UINT64(data + 0xf) != 0 && GET_UINT32(data + 0x7) != 0x80000000)
> --
> 2.14.4
>
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
Elliott, Robert (Servers) Oct. 3, 2018, 2:09 a.m. UTC | #2
> -----Original Message-----
> From: Alex Hung <alex.hung@canonical.com>
> Sent: Tuesday, October 02, 2018 7:33 PM
> To: Elliott, Robert (Persistent Memory) <elliott@hpe.com>
> Cc: fwts-devel <fwts-devel@lists.ubuntu.com>
> Subject: Re: [PATCH] dmicheck: fix handling of type 16 capacities > 2
> TiB
> 
> Hi Robert,
> 
> Thanks for the patch, and this seems to be similar to
> http://patchwork.ozlabs.org/patch/974876/.
> 
> Do you have comments on the other one, ex. yours is more complete?

I think they're functionally the same; that patch is simpler.

An if clause might better facilitate ensuring the structure length
includes the Extended Maximum Capacity field in the 0x8000000 case,
and verifying this rule from the conformance list in annex A:
"Either Maximum Capacity or Extended Maximum Capacity must be set 
to a known, non-zero value."
Alex Hung Oct. 3, 2018, 9:19 a.m. UTC | #3
On Wed, Oct 3, 2018 at 10:09 AM Elliott, Robert (Persistent Memory)
<elliott@hpe.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Alex Hung <alex.hung@canonical.com>
> > Sent: Tuesday, October 02, 2018 7:33 PM
> > To: Elliott, Robert (Persistent Memory) <elliott@hpe.com>
> > Cc: fwts-devel <fwts-devel@lists.ubuntu.com>
> > Subject: Re: [PATCH] dmicheck: fix handling of type 16 capacities > 2
> > TiB
> >
> > Hi Robert,
> >
> > Thanks for the patch, and this seems to be similar to
> > http://patchwork.ozlabs.org/patch/974876/.
> >
> > Do you have comments on the other one, ex. yours is more complete?
>
> I think they're functionally the same; that patch is simpler.
>
> An if clause might better facilitate ensuring the structure length
> includes the Extended Maximum Capacity field in the 0x8000000 case,
> and verifying this rule from the conformance list in annex A:
> "Either Maximum Capacity or Extended Maximum Capacity must be set
> to a known, non-zero value."
>

I personally think the spec is ambiguous about 0x8000000 in Maximum
Capacity. It seems to indicate Extended Maximum Capacity should be
used but also is a valid number for 2TB (Table 70 – Physical Memory
Array (Type 16) structure).

However, 0x8000000 should be a valid value in Maximum Capacity in
either case, and both patches work for me.
Elliott, Robert (Servers) Oct. 3, 2018, 9:34 p.m. UTC | #4
> -----Original Message-----
> From: Alex Hung [mailto:alex.hung@canonical.com]
> Sent: Wednesday, October 3, 2018 4:20 AM
> To: Elliott, Robert (Persistent Memory) <elliott@hpe.com>
> Cc: fwts-devel <fwts-devel@lists.ubuntu.com>
> Subject: Re: [PATCH] dmicheck: fix handling of type 16 capacities > 2 TiB
> 
> On Wed, Oct 3, 2018 at 10:09 AM Elliott, Robert (Persistent Memory)
> <elliott@hpe.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Alex Hung <alex.hung@canonical.com>
> > > Sent: Tuesday, October 02, 2018 7:33 PM
> > > To: Elliott, Robert (Persistent Memory) <elliott@hpe.com>
> > > Cc: fwts-devel <fwts-devel@lists.ubuntu.com>
> > > Subject: Re: [PATCH] dmicheck: fix handling of type 16 capacities > 2
> > > TiB
> > >
> > > Hi Robert,
> > >
> > > Thanks for the patch, and this seems to be similar to
> > > http://patchwork.ozlabs.org/patch/974876/.
> > >
> > > Do you have comments on the other one, ex. yours is more complete?
> >
> > I think they're functionally the same; that patch is simpler.

> > An if clause might better facilitate ensuring the structure length
> > includes the Extended Maximum Capacity field in the 0x8000000 case,
> > and verifying this rule from the conformance list in annex A:
> > "Either Maximum Capacity or Extended Maximum Capacity must be set
> > to a known, non-zero value."
> >
> 
> I personally think the spec is ambiguous about 0x8000000 in Maximum
> Capacity. It seems to indicate Extended Maximum Capacity should be
> used but also is a valid number for 2TB (Table 70 – Physical Memory
> Array (Type 16) structure).

SMBIOS 2.7.0 (2010) introduced the Extended Maximum Capacity field
and added this rule:
"0x80000000 or greater must be represented in the Extended Maximum
Capacity field."

A system compliant with an earlier version wasn't subject to that
rule, so might report a value greater than 0x80000000.  However,
the largest memory capacity per CPU in an x86 server at that time
was 192 GiB, meaning 1.5 TiB for an 8P system; that still doesn't
push this field above 0x80000000.
 
> However, 0x8000000 should be a valid value in Maximum Capacity in
> either case, and both patches work for me.

I confirmed that patch also works on a system with exactly
3 TiB of memory.

Patch
diff mbox series

diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c
index d0096bd4..3733fd98 100644
--- a/src/dmi/dmicheck/dmicheck.c
+++ b/src/dmi/dmicheck/dmicheck.c
@@ -1512,7 +1512,8 @@  static void dmicheck_entry(fwts_framework *fw,
 					data[0x4], table, addr, "Location", 0x4);
 			dmi_min_max_uint8_check(fw, table, addr, "Use", hdr, 0x5, 0x1, 0x7);
 			dmi_min_max_uint8_check(fw, table, addr, "Error Corrrection Type", hdr, 0x6, 0x1, 0x7);
-			dmi_min_max_uint32_check(fw, table, addr, "Maximum Capacity", hdr, 0x7, 0, 0x80000000 - 1);
+			if (GET_UINT32(data + 0x7) != 0x80000000)
+				dmi_min_max_uint32_check(fw, table, addr, "Maximum Capacity", hdr, 0x7, 0, 0x80000000 - 1);
 			if (hdr->length < 0x17)
 				break;
 			if (GET_UINT64(data + 0xf) != 0 && GET_UINT32(data + 0x7) != 0x80000000)