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.

---

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.

index 96c25d9..f1b7f65 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
Gleb.

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.

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

-hpa

Commit-ID:  0c61227094b3ddaca2f847ee287c4a2e3762b5a2
Gitweb:     http://git.kernel.org/tip/0c61227094b3ddaca2f847ee287c4a2e3762b5a2
Author:     Gleb Natapov <[email protected]>
AuthorDate: Tue, 26 Apr 2011 11:21:32 +0300
Committer:  H. Peter Anvin <[email protected]>
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.

---
 drivers/firmware/edd.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

index 96c25d9..f1b7f65 100644
@@ -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;
 }