[PATCH] Fix EDD3.0 data verification.

[PATCH] Fix EDD3.0 data verification.

[Gleb Natapov]

Check for nonzero path in edd_has_edd30() has no sense. First, it looks
at the wrong memory. Device path starts at offset 30 of the info->params
structure which is at offset 8 from the beginning of info structure, but
code looks at info + 4 instead. This was correct when code was introduced,
but around v2.6.4 three more fields were added to edd_info structure
(commit 66b61a5c in history.git). Second, even if it will check correct
memory it will always succeed since at offset 30 (params->key) there will
be non-zero values otherwise previous check would fail.

The patch replaces this bogus check with one that verifies checksum.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
index c501a5b..6c1ac02 100644
--- a/arch/x86/boot/edd.c
+++ b/arch/x86/boot/edd.c
@@ -97,6 +97,13 @@ static int get_edd_info(u8 devno, struct edd_info *ei)
 
 	/* Extended Get Device Parameters */
 
+	/*
+	 * The sum of bytes 30-73 in params structure should be zero after
+	 * int13 call. Set them to 1 to catch the case when bios works
+	 * according to phoenix spec and return 66 bytes. If we left them
+	 * to be zero, checksum will not catch that data is in wrong format.
+	 */
+	memset(&ei->params.key, 1, 74);
 	ei->params.length = sizeof(ei->params);
 	ireg.ah = 0x48;
 	ireg.si = (size_t)&ei->params;
diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index 96c25d9..5e3baac 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -531,8 +531,8 @@ static int
 edd_has_edd30(struct edd_device *edev)
 {
 	struct edd_info *info;
-	int i, nonzero_path = 0;
-	char c;
+	int i;
+	u8 csum = 0;
 
 	if (!edev)
 		return 0;
@@ -544,16 +544,11 @@ edd_has_edd30(struct edd_device *edev)
 		return 0;
 	}
 
-	for (i = 30; i <= 73; i++) {
-		c = *(((uint8_t *) info) + i + 4);
-		if (c) {
-			nonzero_path++;
-			break;
-		}
-	}
-	if (!nonzero_path) {
+	for (i = 30; i <= 73; i++)
+		csum += *(((u8 *)&info->params) + i);
+
+	if (csum)
 		return 0;
-	}
 
 	return 1;
 }
--
			Gleb.

Re: [PATCH] Fix EDD3.0 data verification.

[Henrique de Moraes Holschuh]

On Wed, 02 Feb 2011, Gleb Natapov wrote:
> Check for nonzero path in edd_has_edd30() has no sense. First, it looks
> at the wrong memory. Device path starts at offset 30 of the info->params
> structure which is at offset 8 from the beginning of info structure, but
> code looks at info + 4 instead. This was correct when code was introduced,
> but around v2.6.4 three more fields were added to edd_info structure
> (commit 66b61a5c in history.git). Second, even if it will check correct
> memory it will always succeed since at offset 30 (params->key) there will
> be non-zero values otherwise previous check would fail.

Hmm, would that be a reason for boot lockups on some systems ?  I've
seen that happen on Intel D875PBZ and Debian stable (2.6.26).  We
dropped EDD support then.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Fix EDD3.0 data verification.

[Gleb Natapov]

On Wed, Feb 02, 2011 at 11:30:36AM -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 02 Feb 2011, Gleb Natapov wrote:
> > Check for nonzero path in edd_has_edd30() has no sense. First, it looks
> > at the wrong memory. Device path starts at offset 30 of the info->params
> > structure which is at offset 8 from the beginning of info structure, but
> > code looks at info + 4 instead. This was correct when code was introduced,
> > but around v2.6.4 three more fields were added to edd_info structure
> > (commit 66b61a5c in history.git). Second, even if it will check correct
> > memory it will always succeed since at offset 30 (params->key) there will
> > be non-zero values otherwise previous check would fail.
> 
> Hmm, would that be a reason for boot lockups on some systems ?  I've
> seen that happen on Intel D875PBZ and Debian stable (2.6.26).  We
> dropped EDD support then.
> 
Unlikely. Edd module is not loaded by default and the code that the
patch changes works on in-memory data that was read from BIOS during
boot. It is theoretically possible that BIOS, that works according to
phoenix spec, will do something bad when int13_48 is called with buffer
bigger then 66 bytes, but my patch does not fix that.

--
			Gleb.

Re: [PATCH] Fix EDD3.0 data verification.

[H. Peter Anvin]

On 02/02/2011 03:21 AM, Gleb Natapov wrote:
> Check for nonzero path in edd_has_edd30() has no sense. First, it looks
> at the wrong memory. Device path starts at offset 30 of the info->params
> structure which is at offset 8 from the beginning of info structure, but
> code looks at info + 4 instead. This was correct when code was introduced,
> but around v2.6.4 three more fields were added to edd_info structure
> (commit 66b61a5c in history.git). Second, even if it will check correct
> memory it will always succeed since at offset 30 (params->key) there will
> be non-zero values otherwise previous check would fail.
> 
> The patch replaces this bogus check with one that verifies checksum.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>

This is incorrect; the right thing to do is to use the length byte to
verify the range that should be checksummed.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] Fix EDD3.0 data verification.

[Gleb Natapov]

On Wed, Feb 02, 2011 at 09:14:32AM -0800, H. Peter Anvin wrote:
> On 02/02/2011 03:21 AM, Gleb Natapov wrote:
> > Check for nonzero path in edd_has_edd30() has no sense. First, it looks
> > at the wrong memory. Device path starts at offset 30 of the info->params
> > structure which is at offset 8 from the beginning of info structure, but
> > code looks at info + 4 instead. This was correct when code was introduced,
> > but around v2.6.4 three more fields were added to edd_info structure
> > (commit 66b61a5c in history.git). Second, even if it will check correct
> > memory it will always succeed since at offset 30 (params->key) there will
> > be non-zero values otherwise previous check would fail.
> > 
> > The patch replaces this bogus check with one that verifies checksum.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> 
> This is incorrect; the right thing to do is to use the length byte to
> verify the range that should be checksummed.
> 
According to spec length should be set to 30 on exit.

--
			Gleb.

Re: [PATCH] Fix EDD3.0 data verification.

[H. Peter Anvin]

On 02/02/2011 09:25 AM, Gleb Natapov wrote:
> According to spec length should be set to 30 on exit.

I don't really know how this comment relates to the code, but in the
code I saw a fixed number of bytes being checksummed, which is
definitely wrong.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] Fix EDD3.0 data verification.

[Gleb Natapov]

On Wed, Feb 02, 2011 at 09:29:17AM -0800, H. Peter Anvin wrote:
> On 02/02/2011 09:25 AM, Gleb Natapov wrote:
> > According to spec length should be set to 30 on exit.
> 
> I don't really know how this comment relates to the code, but in the
> code I saw a fixed number of bytes being checksummed, which is
> definitely wrong.
> 
What length do you propose to use? If you were referring to
params->length then it is incorrect since it will be always 30 on exit
and this has nothing to do with size we need to checksum. Spec defines
that the sum of bytes 30-73 should be zero. What is definitely wrong
about this?

--
			Gleb.

Re: [PATCH] Fix EDD3.0 data verification.

[Gleb Natapov]

On Wed, Feb 02, 2011 at 07:38:27PM +0200, Gleb Natapov wrote:
> On Wed, Feb 02, 2011 at 09:29:17AM -0800, H. Peter Anvin wrote:
> > On 02/02/2011 09:25 AM, Gleb Natapov wrote:
> > > According to spec length should be set to 30 on exit.
> > 
> > I don't really know how this comment relates to the code, but in the
> > code I saw a fixed number of bytes being checksummed, which is
> > definitely wrong.
> > 
> What length do you propose to use? If you were referring to
> params->length then it is incorrect since it will be always 30 on exit
> and this has nothing to do with size we need to checksum. Spec defines
> that the sum of bytes 30-73 should be zero. What is definitely wrong
> about this?
> 

Ah I see what length you were referring to params->device_path_info_length.
If we will use that then we will get correct checksum for BIOSes that work
according to phoenix spec too, but edd_show_interface() and edd_show_host_bus()
handle only T13 spec so the information they show can be incorrect. I can
change code to check that params->device_path_info_length == 44 in addition
to checking csum. What do you think?

--
			Gleb.

Re: [PATCH] Fix EDD3.0 data verification.

[H. Peter Anvin]

On 02/02/2011 09:59 AM, Gleb Natapov wrote:
> 
> Ah I see what length you were referring to params->device_path_info_length.
> If we will use that then we will get correct checksum for BIOSes that work
> according to phoenix spec too, but edd_show_interface() and edd_show_host_bus()
> handle only T13 spec so the information they show can be incorrect. I can
> change code to check that params->device_path_info_length == 44 in addition
> to checking csum. What do you think?
> 

Yes, you need to check both.

This is really a union of two similar-but-not-identical structures
distinguished by the length field.

	-hpa

Re: [PATCH] Fix EDD3.0 data verification.

[Gleb Natapov]

On Wed, Feb 02, 2011 at 11:56:55AM -0800, H. Peter Anvin wrote:
> On 02/02/2011 09:59 AM, Gleb Natapov wrote:
> > 
> > Ah I see what length you were referring to params->device_path_info_length.
> > If we will use that then we will get correct checksum for BIOSes that work
> > according to phoenix spec too, but edd_show_interface() and edd_show_host_bus()
> > handle only T13 spec so the information they show can be incorrect. I can
> > change code to check that params->device_path_info_length == 44 in addition
> > to checking csum. What do you think?
> > 
> 
> Yes, you need to check both.
OK.

> 
> This is really a union of two similar-but-not-identical structures
> distinguished by the length field.
> 
Yes, unfortunately phoenix spec lacks some vital information that is
needed to find the device edd entry corresponds too. SCSI target or IDE
channel for instance.
 
--
			Gleb.