compatability with older versions of UEFI

compatability with older versions of UEFI

[Luck, Tony]

When we print UEFI appendix N error logs provided by plaform
firmware we get to this function:

static void cper_estatus_print_section(
	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
{

Which includes this code (similar for the other section types):


	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
		printk("%s""section_type: memory error\n", newpfx);
		if (gdata->error_data_length >= sizeof(*mem_err))
			cper_print_mem(newpfx, mem_err);
		else
			goto err_section_too_small;

I'm having a problem with that size check.  The kernel defines
"struct cper_sec_mem_err" according to the 2.3.1 and later versions
of the spec where the last element is the module handle at offset 78
so the "sizeof(*mem_err)" is 80.  But my BIOS defaults to providing
version 2.2 format information, where the last field is the memory
error type, and the struture size is only 73 bytes. So this code
takes the goto err_section_too_small, prints an error message, and
stops decoding.

This is a pity because it looks like the UEFI folk took great care
to make these structures back/forward compatible. Each has a bitfield
at the start saying which elements are valid ... so my 2.2 using BIOS
will never set the valid bits for the fields it doesn't know about.

Perhaps we can have some defines for the size of these structures
in the oldest version of the spec where they exist (2.1 AFAICT):

#define	UEFI_MEMSECTION_MIN_SIZE	73

etc. Then use these as the sanity check?

Or we can be totally distrustful of the BIOS and pass the
"gdata->error_data_length" to the print function and have
it check that size against the offset of any fields that
the validation_bits say we should print.  This feels like
overkill.

-Tony

Re: compatability with older versions of UEFI

[Zhang, Jonathan Zhixiong]

Another option would be to have 2 structs, the first one
"struct cper_sec_mem_err" holds the structure as defined by UEFI
2.1, the 2nd one "struct cper_sec_mem_err_24_ext" holds the 4
elements added in UEFI 2.3.1.

In the past how is such situation (newer version of spec adds elements
to existing table) handled? To me, the "extension structure" option
proposed above makes sense because it allows rigid check of data
passed from firmware to OS.

Jonathan

On 6/23/2015 10:05 AM, Luck, Tony wrote:
> When we print UEFI appendix N error logs provided by plaform
> firmware we get to this function:
>
> static void cper_estatus_print_section(
> 	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
> {
>
> Which includes this code (similar for the other section types):
>
>
> 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
> 		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
> 		printk("%s""section_type: memory error\n", newpfx);
> 		if (gdata->error_data_length >= sizeof(*mem_err))
> 			cper_print_mem(newpfx, mem_err);
> 		else
> 			goto err_section_too_small;
>
> I'm having a problem with that size check.  The kernel defines
> "struct cper_sec_mem_err" according to the 2.3.1 and later versions
> of the spec where the last element is the module handle at offset 78
> so the "sizeof(*mem_err)" is 80.  But my BIOS defaults to providing
> version 2.2 format information, where the last field is the memory
> error type, and the struture size is only 73 bytes. So this code
> takes the goto err_section_too_small, prints an error message, and
> stops decoding.
>
> This is a pity because it looks like the UEFI folk took great care
> to make these structures back/forward compatible. Each has a bitfield
> at the start saying which elements are valid ... so my 2.2 using BIOS
> will never set the valid bits for the fields it doesn't know about.
>
> Perhaps we can have some defines for the size of these structures
> in the oldest version of the spec where they exist (2.1 AFAICT):
>
> #define	UEFI_MEMSECTION_MIN_SIZE	73
>
> etc. Then use these as the sanity check?
>
> Or we can be totally distrustful of the BIOS and pass the
> "gdata->error_data_length" to the print function and have
> it check that size against the offset of any fields that
> the validation_bits say we should print.  This feels like
> overkill.
>
> -Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Jonathan (Zhixiong) Zhang
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: compatability with older versions of UEFI

[Luck, Tony]

> Another option would be to have 2 structs, the first one
> "struct cper_sec_mem_err" holds the structure as defined by UEFI
> 2.1, the 2nd one "struct cper_sec_mem_err_24_ext" holds the 4
> elements added in UEFI 2.3.1.

Reading some more of the UEFI 2.5 spec ... I see we are in for a world of pain
here.

2.5 adds some small tweaks to the memory structure (adding a couple of extra
bits to the "row" entry that can be grabbed from the formerly reserved byte
at offset 73).  But then there is a whole new GUID for a "Memory Error Section 2"
which has doubled the width of the device, row, column, rank, and bit_pos fields
together with adding two new fields for chip_id and status.  This will be painful
because we hardwired the old sizes into extlog_mem_event in <ras/ras_event.h>

-Tony

Re: compatability with older versions of UEFI

[Zhang, Jonathan Zhixiong]

On 6/24/2015 12:14 PM, Luck, Tony wrote:
>> Another option would be to have 2 structs, the first one
>> "struct cper_sec_mem_err" holds the structure as defined by UEFI
>> 2.1, the 2nd one "struct cper_sec_mem_err_24_ext" holds the 4
>> elements added in UEFI 2.3.1.
>
> Reading some more of the UEFI 2.5 spec ... I see we are in for a world of pain
> here.
>
> 2.5 adds some small tweaks to the memory structure (adding a couple of extra
> bits to the "row" entry that can be grabbed from the formerly reserved byte
> at offset 73).
I think this can be dealt with easily as long as all platforms observe
the rule that if a bit is reserved, the bit should be set a '0' instead
of being set randomly. Is this a fair assumption on all platforms?

> But then there is a whole new GUID for a "Memory Error Section 2"
> which has doubled the width of the device, row, column, rank, and bit_pos fields
> together with adding two new fields for chip_id and status.  This will be painful
> because we hardwired the old sizes into extlog_mem_event in <ras/ras_event.h>
The old size is encoded in "stuct cper_mem_err_compact", other members
of the trace data are the same between "Memory Error Section" and
"Memory Error Section 2". One option we have without having to disturb
user space handler of memory error trace data would be to change
"struct cper_mem_err_compact" so the affected elements are of
__u32 instead of __u16. Drawback of this option is that the trace
buffer will be unnecessarily bigger if a platform generates
"Memory Error Section" data instead of "Memory Error Section 2" data.
Such drawback is not a big issue given that uncorrected memory error
happens infrequently and corrected memory error should be grouped
by platform.

-- 
Jonathan (Zhixiong) Zhang
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: compatability with older versions of UEFI

[Luck, Tony]

> "Memory Error Section 2". One option we have without having to disturb
> user space handler of memory error trace data would be to change
> "struct cper_mem_err_compact" so the affected elements are of
> __u32 instead of __u16. Drawback of this option is that the trace
> buffer will be unnecessarily bigger if a platform generates
> "Memory Error Section" data instead of "Memory Error Section 2" data.

That structure is visible to user level consumers of the event (perhaps just
one of those right now?):

  git://git.fedorahosted.org/rasdaemon.git

We were not as smart as UEFI and didn't include a version number, or other
plan,  that would allow us to transition to a new format cleanly.

Perhaps we could re-purpose some of the high order "validation_bits"
as a version number?  It's a u64 and UEFI 2.5 is only up to bit21 so far,
so perhaps it will be a long, long time before they get that many fields
in some future standard.

-Tony

Re: compatability with older versions of UEFI

[Zhang, Jonathan Zhixiong]

+Mauro

On 6/24/2015 1:12 PM, Luck, Tony wrote:
>> "Memory Error Section 2". One option we have without having to disturb
>> user space handler of memory error trace data would be to change
>> "struct cper_mem_err_compact" so the affected elements are of
>> __u32 instead of __u16. Drawback of this option is that the trace
>> buffer will be unnecessarily bigger if a platform generates
>> "Memory Error Section" data instead of "Memory Error Section 2" data.
>
> That structure is visible to user level consumers of the event (perhaps just
> one of those right now?):
>
>    git://git.fedorahosted.org/rasdaemon.git
>
> We were not as smart as UEFI and didn't include a version number, or other
> plan,  that would allow us to transition to a new format cleanly.
>
> Perhaps we could re-purpose some of the high order "validation_bits"
> as a version number?  It's a u64 and UEFI 2.5 is only up to bit21 so far,
> so perhaps it will be a long, long time before they get that many fields
> in some future standard.
I agree that we could re-purpose some of the high order "validation_bits"
as a version number. That being said, I am not sure whether that
approach is preferred by user space tool authors. My feeling is
such approach would make user space tool more complicated (eg.
has to understand versions). Mauro, pls. correct me is I am wrong;
pls. refer to previous email in this thread for context related to
challenge brought forth by UEFI 2.5.

perf interface has debugfs interface, so if user space tool does
following, compatibility with different kernel version and different
spec version will be maintained:
* Use debugfs interface to discover format of trace data.
* use largest size known for user space structure; check size of member
in user space structure and size of corresponding field in trace data,
adjust data as necessary.

In the mean time, I think when kernel defines TRACE_EVENT, it should
try not having to use __field_struct, that would make format inside that
field opaque to user space tool. For instance:
   __field_struct(struct cper_mem_err_compact, data)
Because of above, ras-extlog-handler.c in rasdaemon has to hardcode
this structure, making it harder to maintain forward/backward
compatibility.

-- 
Jonathan (Zhixiong) Zhang
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: compatability with older versions of UEFI

[Matt Fleming]

On Wed, 24 Jun, at 11:04:05AM, Zhang, Jonathan Zhixiong wrote:
> Another option would be to have 2 structs, the first one
> "struct cper_sec_mem_err" holds the structure as defined by UEFI
> 2.1, the 2nd one "struct cper_sec_mem_err_24_ext" holds the 4
> elements added in UEFI 2.3.1.

Yes, this is traditionally how we've handled this issue in other parts
of the kernel that deal with UEFI/ACPI.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: compatability with older versions of UEFI

[Matt Fleming]

On Wed, 24 Jun, at 07:14:43PM, Luck, Tony wrote:
> > Another option would be to have 2 structs, the first one
> > "struct cper_sec_mem_err" holds the structure as defined by UEFI
> > 2.1, the 2nd one "struct cper_sec_mem_err_24_ext" holds the 4
> > elements added in UEFI 2.3.1.
> 
> Reading some more of the UEFI 2.5 spec ... I see we are in for a world of pain
> here.
> 
> 2.5 adds some small tweaks to the memory structure (adding a couple of extra
> bits to the "row" entry that can be grabbed from the formerly reserved byte
> at offset 73).  But then there is a whole new GUID for a "Memory Error Section 2"
> which has doubled the width of the device, row, column, rank, and bit_pos fields
> together with adding two new fields for chip_id and status.  This will be painful
> because we hardwired the old sizes into extlog_mem_event in <ras/ras_event.h>

Can't you just create a new memory event type? Granted, you'll need
changes to the ras daemon, but since there's a new GUID involved I think
that's forgivable.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH] efi: Handle memory error structures produced based on old versions of standard

[Luck, Tony]

The memory error record structure includes as its first field a
bitmask of which subsequent fields are valid. The allows new fields
to be added to the structure while keeping compatibility with older
software that parses these records. This mechanism was used between
versions 2.2 and 2.3 to add four new fields, growing the size of the
structure from 73 bytes to 80. But Linux just added all the new
fields so this test:
	if (gdata->error_data_length >= sizeof(*mem_err))
		cper_print_mem(newpfx, mem_err);
	else
		goto err_section_too_small;
now make Linux complain about old format records being too short.

Add a definition for the old format of the structure and use that
for the minimum size check. Pass the actual size to cper_print_mem()
so it can sanity check the validation_bits field to ensure that if
a BIOS using the old format sets bits as if it were new, we won't
access fields beyond the end of the structure.

Signed-off-by: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/cper.c | 13 ++++++++++---
 include/linux/cper.h        | 22 +++++++++++++++++++++-
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 4fd9961d552e..b2012004a8ab 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -305,10 +305,15 @@ const char *cper_mem_err_unpack(struct trace_seq *p,
 	return ret;
 }
 
-static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
+static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem,
+	int len)
 {
 	struct cper_mem_err_compact cmem;
 
+	/* Don't trust UEFI 2.1/2.2 structure with bad validation bits */
+	if (len == sizeof(struct cper_sec_mem_err_old) &&
+	    (mem->validation_bits & ~(CPER_MEM_VALID_RANK_NUMBER - 1)))
+		return;
 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
 		printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status);
 	if (mem->validation_bits & CPER_MEM_VALID_PA)
@@ -405,8 +410,10 @@ static void cper_estatus_print_section(
 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
 		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
 		printk("%s""section_type: memory error\n", newpfx);
-		if (gdata->error_data_length >= sizeof(*mem_err))
-			cper_print_mem(newpfx, mem_err);
+		if (gdata->error_data_length >=
+		    sizeof(struct cper_sec_mem_err_old))
+			cper_print_mem(newpfx, mem_err,
+				       gdata->error_data_length);
 		else
 			goto err_section_too_small;
 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) {
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 76abba4b238e..dcacb1a72e26 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -340,7 +340,27 @@ struct cper_ia_proc_ctx {
 	__u64	mm_reg_addr;
 };
 
-/* Memory Error Section */
+/* Old Memory Error Section UEFI 2.1, 2.2 */
+struct cper_sec_mem_err_old {
+	__u64	validation_bits;
+	__u64	error_status;
+	__u64	physical_addr;
+	__u64	physical_addr_mask;
+	__u16	node;
+	__u16	card;
+	__u16	module;
+	__u16	bank;
+	__u16	device;
+	__u16	row;
+	__u16	column;
+	__u16	bit_pos;
+	__u64	requestor_id;
+	__u64	responder_id;
+	__u64	target_id;
+	__u8	error_type;
+};
+
+/* Memory Error Section UEFI >= 2.3 */
 struct cper_sec_mem_err {
 	__u64	validation_bits;
 	__u64	error_status;
-- 
2.1.4

Re: [PATCH] efi: Handle memory error structures produced based on old versions of standard

[Matt Fleming]

On Mon, 29 Jun, at 11:21:07AM, Luck, Tony wrote:
> The memory error record structure includes as its first field a
> bitmask of which subsequent fields are valid. The allows new fields
> to be added to the structure while keeping compatibility with older
> software that parses these records. This mechanism was used between
> versions 2.2 and 2.3 to add four new fields, growing the size of the
> structure from 73 bytes to 80. But Linux just added all the new
> fields so this test:
> 	if (gdata->error_data_length >= sizeof(*mem_err))
> 		cper_print_mem(newpfx, mem_err);
> 	else
> 		goto err_section_too_small;
> now make Linux complain about old format records being too short.
> 
> Add a definition for the old format of the structure and use that
> for the minimum size check. Pass the actual size to cper_print_mem()
> so it can sanity check the validation_bits field to ensure that if
> a BIOS using the old format sets bits as if it were new, we won't
> access fields beyond the end of the structure.
> 
> Signed-off-by: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/cper.c | 13 ++++++++++---
>  include/linux/cper.h        | 22 +++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 4fd9961d552e..b2012004a8ab 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -305,10 +305,15 @@ const char *cper_mem_err_unpack(struct trace_seq *p,
>  	return ret;
>  }
>  
> -static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> +static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem,
> +	int len)
>  {
>  	struct cper_mem_err_compact cmem;
>  
> +	/* Don't trust UEFI 2.1/2.2 structure with bad validation bits */
> +	if (len == sizeof(struct cper_sec_mem_err_old) &&
> +	    (mem->validation_bits & ~(CPER_MEM_VALID_RANK_NUMBER - 1)))
> +		return;

I would have thought that we'd want to print an error message here
instead of silently returning?

Otherwise looks good.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCHv2] efi: Handle memory error structures produced based on old versions of standard

[Luck, Tony]

The memory error record structure includes as its first field a
bitmask of which subsequent fields are valid. The allows new fields
to be added to the structure while keeping compatibility with older
software that parses these records. This mechanism was used between
versions 2.2 and 2.3 to add four new fields, growing the size of the
structure from 73 bytes to 80. But Linux just added all the new
fields so this test:
	if (gdata->error_data_length >= sizeof(*mem_err))
		cper_print_mem(newpfx, mem_err);
	else
		goto err_section_too_small;
now make Linux complain about old format records being too short.

Add a definition for the old format of the structure and use that
for the minimum size check. Pass the actual size to cper_print_mem()
so it can sanity check the validation_bits field to ensure that if
a BIOS using the old format sets bits as if it were new, we won't
access fields beyond the end of the structure.

Signed-off-by: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
v1-v2: print FW_WARN if we see bogus validation bits from an old sized structure

 drivers/firmware/efi/cper.c | 15 ++++++++++++---
 include/linux/cper.h        | 22 +++++++++++++++++++++-
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 4fd9961d552e..d42537425438 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -305,10 +305,17 @@ const char *cper_mem_err_unpack(struct trace_seq *p,
 	return ret;
 }
 
-static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
+static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem,
+	int len)
 {
 	struct cper_mem_err_compact cmem;
 
+	/* Don't trust UEFI 2.1/2.2 structure with bad validation bits */
+	if (len == sizeof(struct cper_sec_mem_err_old) &&
+	    (mem->validation_bits & ~(CPER_MEM_VALID_RANK_NUMBER - 1))) {
+		pr_err(FW_WARN "valid bits set for fields beyond structure\n");
+		return;
+	}
 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
 		printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status);
 	if (mem->validation_bits & CPER_MEM_VALID_PA)
@@ -405,8 +412,10 @@ static void cper_estatus_print_section(
 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
 		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
 		printk("%s""section_type: memory error\n", newpfx);
-		if (gdata->error_data_length >= sizeof(*mem_err))
-			cper_print_mem(newpfx, mem_err);
+		if (gdata->error_data_length >=
+		    sizeof(struct cper_sec_mem_err_old))
+			cper_print_mem(newpfx, mem_err,
+				       gdata->error_data_length);
 		else
 			goto err_section_too_small;
 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) {
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 76abba4b238e..dcacb1a72e26 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -340,7 +340,27 @@ struct cper_ia_proc_ctx {
 	__u64	mm_reg_addr;
 };
 
-/* Memory Error Section */
+/* Old Memory Error Section UEFI 2.1, 2.2 */
+struct cper_sec_mem_err_old {
+	__u64	validation_bits;
+	__u64	error_status;
+	__u64	physical_addr;
+	__u64	physical_addr_mask;
+	__u16	node;
+	__u16	card;
+	__u16	module;
+	__u16	bank;
+	__u16	device;
+	__u16	row;
+	__u16	column;
+	__u16	bit_pos;
+	__u64	requestor_id;
+	__u64	responder_id;
+	__u64	target_id;
+	__u8	error_type;
+};
+
+/* Memory Error Section UEFI >= 2.3 */
 struct cper_sec_mem_err {
 	__u64	validation_bits;
 	__u64	error_status;
-- 
2.1.4

Re: [PATCHv2] efi: Handle memory error structures produced based on old versions of standard

[Matt Fleming]

On Tue, 30 Jun, at 03:57:51PM, Luck, Tony wrote:
> The memory error record structure includes as its first field a
> bitmask of which subsequent fields are valid. The allows new fields
> to be added to the structure while keeping compatibility with older
> software that parses these records. This mechanism was used between
> versions 2.2 and 2.3 to add four new fields, growing the size of the
> structure from 73 bytes to 80. But Linux just added all the new
> fields so this test:
> 	if (gdata->error_data_length >= sizeof(*mem_err))
> 		cper_print_mem(newpfx, mem_err);
> 	else
> 		goto err_section_too_small;
> now make Linux complain about old format records being too short.
> 
> Add a definition for the old format of the structure and use that
> for the minimum size check. Pass the actual size to cper_print_mem()
> so it can sanity check the validation_bits field to ensure that if
> a BIOS using the old format sets bits as if it were new, we won't
> access fields beyond the end of the structure.
> 
> Signed-off-by: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> v1-v2: print FW_WARN if we see bogus validation bits from an old sized structure
> 
>  drivers/firmware/efi/cper.c | 15 ++++++++++++---
>  include/linux/cper.h        | 22 +++++++++++++++++++++-
>  2 files changed, 33 insertions(+), 4 deletions(-)

Looks good to me Tony. Since this is essentially a bug fix, does it need
applying to stable?

-- 
Matt Fleming, Intel Open Source Technology Center

RE: [PATCHv2] efi: Handle memory error structures produced based on old versions of standard

[Luck, Tony]

> Looks good to me Tony. Since this is essentially a bug fix, does it need
> applying to stable?

Yes.  Please add a Cc: stable tag when applying

Thanks for catching that

-Tony