Fix EDD3.0 data verification.
PATCH 1/1 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.
arch/x86/boot/edd.c +7 -0
@@ -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;
drivers/firmware/edd.c +6 -11
@@ -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.
Quoted 8 lines · 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 theinfo->paramsstructure 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 toedd_infostructure (commit66b61a5cin 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.
Signature · 4 lines
"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
Quoted 10 lines · Henrique de Moraes Holschuh
Quoted 8 lines · 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 theinfo->paramsstructure 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 toedd_infostructure (commit66b61a5cin 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.
Quoted 4 lines
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.
H. Peter Anvin
reviewerQuoted 9 lines · 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 theinfo->paramsstructure 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 toedd_infostructure (commit66b61a5cin 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.
Quoted 3 lines
The patch replaces this bogus check with one that verifies checksum.
Signed-off-by Gleb Natapov <[email protected]>
This is incorrect; the right thing to do is to use the length byte to verify the range that should be checksummed.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Quoted 10 lines · H. Peter Anvin
Quoted 9 lines · 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 theinfo->paramsstructure 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 toedd_infostructure (commit66b61a5cin 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.
Quoted 7 lines
Quoted 3 lines
The patch replaces this bogus check with one that verifies checksum.
Signed-off-by Gleb Natapov <[email protected]>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.
H. Peter Anvin
reviewerQuoted 1 line · Gleb Natapov
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
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Quoted 7 lines · H. Peter Anvin
Quoted 1 line · Gleb Natapov
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.
Quoted 8 lines · Gleb Natapov
Quoted 7 lines · H. Peter Anvin
Quoted 1 line · Gleb Natapov
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.
Quoted 6 lines
What length do you propose to use? If you were referring to
params->lengththen 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.
H. Peter Anvin
reviewerQuoted 8 lines · Gleb Natapov
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, butedd_show_interface()andedd_show_host_bus()handle only T13 spec so the information they show can be incorrect. I can change code to check thatparams->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
Quoted 11 lines · H. Peter Anvin
Quoted 8 lines · Gleb Natapov
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, butedd_show_interface()andedd_show_host_bus()handle only T13 spec so the information they show can be incorrect. I can change code to check thatparams->device_path_info_length== 44 in addition to checking csum. What do you think?Yes, you need to check both.
OK.
Quoted 4 lines
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.