[PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[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.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 drivers/firmware/dmi_scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 099d83e4e910..2ed51651565f 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -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;
 }
-- 
2.11.0

Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > 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.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Sorry, this should have had a:

Cc: stable@vger.kernel.org

As 4.19 is affected. Jean, can you add that when committing?


> ---
 >  drivers/firmware/dmi_scan.c | 4 ++--
 >  1 file changed, 2 insertions(+), 2 deletions(-)

 > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
 > index 099d83e4e910..2ed51651565f 100644
 > --- a/drivers/firmware/dmi_scan.c
 > +++ b/drivers/firmware/dmi_scan.c
 > @@ -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;
 >  }
 > -- 
 > 2.11.0


-- 
Bye, Peter Korsgaard

Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[Jean Delvare]

On Wed, 2018-12-05 at 22:13 +0100, Peter Korsgaard wrote:
> 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.

> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
> ---
>  drivers/firmware/dmi_scan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 099d83e4e910..2ed51651565f 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -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;
>  }

Nak. This is too late. Changing it again would just add confusion.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[Peter Korsgaard]

>>>>> "Jean" == Jean Delvare <jdelvare@suse.com> writes:

 > On Wed, 2018-12-05 at 22:13 +0100, Peter Korsgaard wrote:
 >> 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.

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.


 > 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.

 > 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.

-- 
Bye, Peter Korsgaard

Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[Jean Delvare]

On Thu, 2018-12-06 at 10:22 +0100, Peter Korsgaard wrote:
> > > > > > "Jean" == Jean Delvare <jdelvare@suse.com> writes:
> 
>  > On Wed, 2018-12-05 at 22:13 +0100, Peter Korsgaard wrote:
>  >> 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.
> 
> 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".

> 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.

>  > 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?

>  > 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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[Peter Korsgaard]

>>>>> "Jean" == Jean Delvare <jdelvare@suse.com> writes:

Hi,

 >> 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 :/


 > 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.


 >> 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.


 > 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.



 >> > 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.


 > 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.



 >> > 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).


 > 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.

-- 
Bye, Peter Korsgaard

Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

Hi Jean,

 >> 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?

-- 
Bye, Peter Korsgaard

Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[Jean Delvare]

On Tue, 2018-12-11 at 13:06 +0100, Peter Korsgaard wrote:
> > > > > > "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:
> 
> Hi Jean,
> 
>  >> 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?

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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] Revert "firmware: dmi_scan: Use lowercase letters for UUID"

[Peter Korsgaard]

>>>>> "Jean" == Jean Delvare <jdelvare@suse.com> writes:

 > On Tue, 2018-12-11 at 13:06 +0100, Peter Korsgaard wrote:
 >> > > > > > "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:
 >> 
 >> Hi Jean,
 >> 
 >> >> 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?

 > 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?

-- 
Bye, Peter Korsgaard