Revert "firmware: dmi_scan: Use lowercase letters for UUID"
PATCH 1/1 Revert "firmware: dmi_scan: Use lowercase letters for UUID"
This reverts commit 712ff25450bd01366301eef81c33e865d901e7b7.
The output of dmi_save_uuid() is exposed to user space as /sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards compatibility, E.G. I have systems that include the content of dmi/id/product_uuid as part of the keyphrase for cryptsetup luksOpen.
As the change was purely cosmetical, revert it to fix such breakage.
--- drivers/firmware/dmi_scan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/firmware/dmi_scan.c +2 -2
@@ -211,9 +211,9 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,* says that this is the defacto standard. */ if (dmi_ver >=0x020600 ) -sprintf (s,"%pUl" , d); +sprintf (s,"%pUL" , d);else -sprintf (s,"%pUb" , d); +sprintf (s,"%pUB" , d); dmi_ident[slot] = s; }
Signature · 1 line
2.11.0
Quoted 11 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Peter" == Peter Korsgaard <[email protected]> writes:
This reverts commit
712ff25450bd01366301eef81c33e865d901e7b7. The output ofdmi_save_uuid()is exposed to user space as/sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards compatibility, E.G. I have systems that include the content of dmi/id/product_uuid as part of the keyphrase for cryptsetup luksOpen.As the change was purely cosmetical, revert it to fix such breakage.
Signed-off-by Peter Korsgaard <[email protected]>
Sorry, this should have had a:
As 4.19 is affected. Jean, can you add that when committing?
Quoted 22 lines
--- drivers/firmware/dmi_scan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/firmware/dmi_scan.c +2
-2@@ -211,9 +211,9 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,* says that this is the defacto standard. */ if (dmi_ver >=0x020600 ) -sprintf (s,"%pUl" , d); +sprintf (s,"%pUL" , d);else -sprintf (s,"%pUb" , d); +sprintf (s,"%pUB" , d); dmi_ident[slot] = s; }Signature · 1 line
2.11.0
Signature · 1 line
Bye, Peter Korsgaard
Jean Delvare
maintainerQuoted 8 lines · Peter Korsgaard
This reverts commit
712ff25450bd01366301eef81c33e865d901e7b7.The output of
dmi_save_uuid()is exposed to user space as/sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards compatibility, E.G. I have systems that include the content of dmi/id/product_uuid as part of the keyphrase for cryptsetup luksOpen.As the change was purely cosmetical, revert it to fix such breakage.
The change is not "cosmetical". The change was done to comply with RFC 4122:
https://tools.ietf.org/html/rfc4122
The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.
If "cryptsetup luksOpen" does not lowercase digits before computing its key passphrase, then it's not RFC 4122-compliant and should be fixed.
Quoted 11 lines
Signed-off-by Peter Korsgaard <[email protected]>--- drivers/firmware/dmi_scan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/firmware/dmi_scan.c +0
-0@@ -211,9 +211,9 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,* says that this is the defacto standard.
drivers/firmware/dmi_scan.c static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, · line 219
Quoted 10 lines
drivers/firmware/dmi_scan.c +2
-2*/ if (dmi_ver >=0x020600 ) -sprintf (s,"%pUl" , d); +sprintf (s,"%pUL" , d);else -sprintf (s,"%pUb" , d); +sprintf (s,"%pUB" , d); dmi_ident[slot] = s; }
Nak. This is too late. Changing it again would just add confusion.
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 12 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Jean" == Jean Delvare <[email protected]> writes:
Quoted 8 lines · Peter Korsgaard
This reverts commit
712ff25450bd01366301eef81c33e865d901e7b7.The output of
dmi_save_uuid()is exposed to user space as/sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards compatibility, E.G. I have systems that include the content of dmi/id/product_uuid as part of the keyphrase for cryptsetup luksOpen.As the change was purely cosmetical, revert it to fix such breakage.
Quoted 7 lines
The change is not "cosmetical". The change was done to comply with RFC 4122:
https://tools.ietf.org/html/rfc4122
The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.
I get that - but it changes the content of sysfs entries, breaking real systems - E.G. a user space ABI regression.
It is a cosmetic code change in the sense that no known software was broken with the upper case characters.
Quoted 2 lines
If "cryptsetup luksOpen" does not lowercase digits before computing its key passphrase, then it's not RFC 4122-compliant and should be fixed.
cryptsetup naturally doesn't know anything about RFC 4122. It just reads a disk encryption keyphrase which happen to include the content of id/product_uuid because of my scripts.
Quoted 1 line
Nak. This is too late. Changing it again would just add confusion.
Please reconsider. 4.17 is from June, and 4.19 has only recently become LTS.
Signature · 1 line
Bye, Peter Korsgaard
Jean Delvare
maintainerQuoted 15 lines · Peter Korsgaard
Quoted 14 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Jean" == Jean Delvare <[email protected]> writes:
Quoted 8 lines · Peter Korsgaard
This reverts commit
712ff25450bd01366301eef81c33e865d901e7b7.The output of
dmi_save_uuid()is exposed to user space as/sys/devices/virtual/dmi/id/*_uuid, so this breaks backwards compatibility, E.G. I have systems that include the content of dmi/id/product_uuid as part of the keyphrase for cryptsetup luksOpen.As the change was purely cosmetical, revert it to fix such breakage.
The change is not "cosmetical". The change was done to comply with RFC 4122:
Quoted 7 lines
Quoted 4 lines
https://tools.ietf.org/html/rfc4122
The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.
I get that - but it changes the content of sysfs entries, breaking real systems - E.G. a user space ABI regression.
I know it's very convenient to play the "user-space ABI regression" card whenever you want a change reverted. However the interface itself did not change. The sysfs file name did not change, the nature of its contents did not change. The only thing which changed is the exact contents details of the file. In my opinion that does not qualify as an "ABI regression".
Quoted 2 lines
It is a cosmetic code change in the sense that no known software was broken with the upper case characters.
It bothered someone enough to create a ticket asking me to fix it in dmidecode:
https://savannah.nongnu.org/bugs/index.php?53569
And it wouldn't make sense that the kernel uses a different format from dmidecode.
Quoted 6 lines
Quoted 2 lines
If "cryptsetup luksOpen" does not lowercase digits before computing its key passphrase, then it's not RFC 4122-compliant and should be fixed.
cryptsetup naturally doesn't know anything about RFC 4122. It just reads a disk encryption keyphrase which happen to include the content of id/product_uuid because of my scripts.
OK, so basically your script infringes RFC 4122, and instead of fixing that, you ask me to stop respecting that RFC on my side.
Out of curiosity, what's the purpose of your encryption strategy? That someone who would open your computer and steal only the disk (as opposed to stealing the whole computer) would be unable to read the disk's contents? Do thieves really do that?
Quoted 4 lines
Quoted 1 line
Nak. This is too late. Changing it again would just add confusion.
Please reconsider. 4.17 is from June, and 4.19 has only recently become LTS.
I can't see how this is related with kernel 4.19 becoming LTS.
My patch has been public since April 8th, 2018. It has been in 3 official kernel versions already. I did not hear any complaint about that change in 8 months. You are the first one to complain, and that's about a custom script that's not RFC-compliant and that you could fix with a one-liner.
Look, you can imagine that I was perfectly aware of what I was doing when I made that change, and that I pondered the decision carefully at that time. And my decision was that the change should be made. As far as I'm concerned, this ship has sailed already, sorry.
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Jean" == Jean Delvare <[email protected]> writes:
Hi,
Quoted 5 lines
Quoted 2 lines
I get that - but it changes the content of sysfs entries, breaking real systems - E.G. a user space ABI regression.
I know it's very convenient to play the "user-space ABI regression" card whenever you want a change reverted.
Sorry, I am really not trying to be difficult here, but that is exactly what it is. A kernel upgrade broke working systems :/
Quoted 4 lines
However the interface itself did not change. The sysfs file name did not change, the nature of its contents did not change. The only thing which changed is the exact contents details of the file. In my opinion that does not qualify as an "ABI regression".
The binary content of the sysfs file changed, and the change caused working software to no longer work.
I get that my software is is not very commonly used. If this change would have broken ps or top or any other well known utility looking in /proc or /sys I guess we wouldn't have this discussion at all, but it is still a regression.
Quoted 6 lines
Quoted 2 lines
It is a cosmetic code change in the sense that no known software was broken with the upper case characters.
It bothered someone enough to create a ticket asking me to fix it in dmidecode:
https://savannah.nongnu.org/bugs/index.php?53569
Yes, I am aware of that.
Quoted 2 lines
And it wouldn't make sense that the kernel uses a different format from dmidecode.
I would personally be quite wary of such change for both dmidecode and the kernel because of the risk of breaking existing utilities. I see that Petter Reinholdtsen has the same concerns:
https://lists.nongnu.org/archive/html/dmidecode-devel/2018-04/msg00001.html
But ok, not my choice to make. I also get that dmidecode doesn't have the same no-regression policy for its output as the kernel has.
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
If "cryptsetup luksOpen" does not lowercase digits before computing its key passphrase, then it's not RFC 4122-compliant and should be fixed.
cryptsetup naturally doesn't know anything about RFC 4122. It just reads a disk encryption keyphrase which happen to include the content of id/product_uuid because of my scripts.
OK, so basically your script infringes RFC 4122, and instead of fixing that, you ask me to stop respecting that RFC on my side.
To be clear, the RFC states:
The formal definition of the UUID string representation is provided by the following ABNF:
hexDigit =
"0" / "1" / "2" / "3" / "4" / "5" / "6" / "7" / "8" / "9" /
"a" / "b" / "c" / "d" / "e" / "f" /
"A" / "B" / "C" / "D" / "E" / "F"(E.G. lower case AND upper case)
And:
The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.
So in other words, no conforming implementation should have any issues handling an upper case UUID string, and the change to lower case was for cosmetic reasons rather than to fix a parsing issue with conforming software.
Quoted 4 lines
Out of curiosity, what's the purpose of your encryption strategy? That someone who would open your computer and steal only the disk (as opposed to stealing the whole computer) would be unable to read the disk's contents? Do thieves really do that?
The systems are locked down, so they cannot (easily) be tricked into revealing their secrets at runtime or boot non-trusted software. The disk encryption protects against somebody moving the disk to another machine and reading the secrets.
I agree that a better solution may be to store the per-device key in E.G. a TPM instead, but that was not available for all use cases.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Nak. This is too late. Changing it again would just add confusion.
Please reconsider. 4.17 is from June, and 4.19 has only recently become LTS.
I can't see how this is related with kernel 4.19 becoming LTS.
Only in the sense that that LTS kernels see wider use than non-LTS kernels (E.G. I discovered this when moving from 4.14.x to 4.19.x).
Quoted 4 lines
Look, you can imagine that I was perfectly aware of what I was doing when I made that change, and that I pondered the decision carefully at that time. And my decision was that the change should be made. As far as I'm concerned, this ship has sailed already, sorry.
Sorry, what is the perceived risk of reverting this change? Just the minor inconsistency between the dmidecode and sysfs output? As stated above, the RFC requires conforming parsers to handle upper case as well.
Signature · 1 line
Bye, Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Peter" == Peter Korsgaard <[email protected]> writes:
Hi Jean,
Quoted 8 lines
Quoted 4 lines
Look, you can imagine that I was perfectly aware of what I was doing when I made that change, and that I pondered the decision carefully at that time. And my decision was that the change should be made. As far as I'm concerned, this ship has sailed already, sorry.
Sorry, what is the perceived risk of reverting this change? Just the minor inconsistency between the dmidecode and sysfs output? As stated above, the RFC requires conforming parsers to handle upper case as well.
I would appreciate if you could explain what risk you see from reverting this change?
Signature · 1 line
Bye, Peter Korsgaard
Jean Delvare
maintainerQuoted 9 lines · Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Peter" == Peter Korsgaard <[email protected]> writes:
Hi Jean,
Quoted 4 lines
Quoted 4 lines
Look, you can imagine that I was perfectly aware of what I was doing when I made that change, and that I pondered the decision carefully at that time. And my decision was that the change should be made. As far as I'm concerned, this ship has sailed already, sorry.
Quoted 6 lines
Quoted 3 lines
Sorry, what is the perceived risk of reverting this change? Just the minor inconsistency between the dmidecode and sysfs output? As stated above, the RFC requires conforming parsers to handle upper case as well.
I would appreciate if you could explain what risk you see from reverting this change?
The exact same risk that you are complaining about, for a different pair of kernel versions. You cannot at the same time argue that the change should not have been done back then, and ask for same change to be done again now.
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 16 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Jean" == Jean Delvare <[email protected]> writes:
Quoted 13 lines · Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Peter" == Peter Korsgaard <[email protected]> writes:
Hi Jean,
Quoted 8 lines
Quoted 4 lines
Look, you can imagine that I was perfectly aware of what I was doing when I made that change, and that I pondered the decision carefully at that time. And my decision was that the change should be made. As far as I'm concerned, this ship has sailed already, sorry.
Sorry, what is the perceived risk of reverting this change? Just the minor inconsistency between the dmidecode and sysfs output? As stated above, the RFC requires conforming parsers to handle upper case as well.
Quoted 7 lines
Quoted 2 lines
I would appreciate if you could explain what risk you see from reverting this change?
The exact same risk that you are complaining about, for a different pair of kernel versions. You cannot at the same time argue that the change should not have been done back then, and ask for same change to be done again now.
With that kind of catch-22 logic, no regressions can ever be fixed. This change was part of 4.17, released 6 months ago, whereas the previous behaviour has existed for an order of magnitude longer.
While it is true that there is a chance that somebody may rely on the new behaviour, it is likely to be significantly smaller than the chance that someone relied on the previous behaviour (E.G. the breakage in my software is proof of at least one such instance). Given that 4.19 has only recently become a LTS kernel and distibutions with 4.17+ are only getting released now (Fedora 29, Ubuntu 18.10) chances are that more people will be affected in the future.
But you are right, we should do the revert as soon as possible, before people start relying on this new behaviour.
I can extend the commit message with a reference to RFC4122 if you prefer that over my "the change was purely cosmetical" wording?
Signature · 1 line
Bye, Peter Korsgaard