[PATCH][RESEND] Fix EDD3.0 data verification.

[PATCH][RESEND] 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>
---

Here https://lkml.org/lkml/2011/2/16/305 Peter said he will pull it into
the tree, but I do not see this patch applied anywhere.

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index 96c25d9..f1b7f65 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,16 @@ 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) {
+
+	/* We support only T13 spec */
+	if (info->params.device_path_info_length != 44)
+		return 0;
+
+	for (i = 30; i < info->params.device_path_info_length + 30; i++)
+		csum += *(((u8 *)&info->params) + i);
+
+	if (csum)
 		return 0;
-	}
 
 	return 1;
 }
--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

--
			Gleb.

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

[H. Peter Anvin]

On 04/26/2011 01:21 AM, Gleb Natapov wrote:
> +
> +	/* We support only T13 spec */
> +	if (info->params.device_path_info_length != 44)
> +		return 0;
> +

Please make it work correctly with both versions of the structure, instead.

	-hpa

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

[Gleb Natapov]

On Tue, Apr 26, 2011 at 03:42:52PM -0700, H. Peter Anvin wrote:
> On 04/26/2011 01:21 AM, Gleb Natapov wrote:
> > +
> > +	/* We support only T13 spec */
> > +	if (info->params.device_path_info_length != 44)
> > +		return 0;
> > +
> 
> Please make it work correctly with both versions of the structure, instead.
> 
Current Linux EDD support is written to conform to T13 standard as it
says in the file header:

 * BIOS Enhanced Disk Drive Services (EDD)
 * conformant to T13 Committee www.t13.org
 *   projects 1572D, 1484D, 1386D, 1226DT

This patch is only fixes obviously incorrect code in edd_has_edd30()
to do proper check instead of summing random memory and it should be
applied even if we decide that we want to support Phoenix spec too.

I looked into implementing support for Phoenix spec back then when I
wrote the patch and I decided against it. The main (and may be only)
use case for edd module is to provide OS installer with enough info
for it to decide where to install boot loader. Unfortunately Phoenix
EDD spec does not support most modern storage technologies and even for
legacy one such as ATA it does not provide enough info to determine boot
disk correctly (you can't tell the difference between two ATA channels,
T13 spec provides this info in interface path along with PCI address).
So even if we would have support for it the OS installer will have to
check if the information comes from T13 or Phoenix and ignore it if it's
the later.

--
			Gleb.

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

[H. Peter Anvin]

On 04/27/2011 05:32 AM, Gleb Natapov wrote:
> 
> This patch is only fixes obviously incorrect code in edd_has_edd30()
> to do proper check instead of summing random memory and it should be
> applied even if we decide that we want to support Phoenix spec too.
> 
> I looked into implementing support for Phoenix spec back then when I
> wrote the patch and I decided against it. The main (and may be only)
> use case for edd module is to provide OS installer with enough info
> for it to decide where to install boot loader. Unfortunately Phoenix
> EDD spec does not support most modern storage technologies and even for
> legacy one such as ATA it does not provide enough info to determine boot
> disk correctly (you can't tell the difference between two ATA channels,
> T13 spec provides this info in interface path along with PCI address).
> So even if we would have support for it the OS installer will have to
> check if the information comes from T13 or Phoenix and ignore it if it's
> the later.
> 

I still think it would be a lot better to let userspace decide how it
wants to handle things, but oh well.

	-hpa

[tip:x86/setup] x86, setup: Fix EDD3.0 data verification.

[tip-bot for Gleb Natapov]

Commit-ID:  0c61227094b3ddaca2f847ee287c4a2e3762b5a2
Gitweb:     http://git.kernel.org/tip/0c61227094b3ddaca2f847ee287c4a2e3762b5a2
Author:     Gleb Natapov <gleb@redhat.com>
AuthorDate: Tue, 26 Apr 2011 11:21:32 +0300
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed, 27 Apr 2011 14:16:12 -0700

x86, setup: Fix EDD3.0 data verification.

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>
Link: http://lkml.kernel.org/r/20110426082132.GG2265@redhat.com
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 drivers/firmware/edd.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index 96c25d9..f1b7f65 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,16 @@ 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) {
+
+	/* We support only T13 spec */
+	if (info->params.device_path_info_length != 44)
+		return 0;
+
+	for (i = 30; i < info->params.device_path_info_length + 30; i++)
+		csum += *(((u8 *)&info->params) + i);
+
+	if (csum)
 		return 0;
-	}
 
 	return 1;
 }