[PATCH] libfdisk: (gpt) fix attributes endianess

[PATCH] libfdisk: (gpt) fix attributes endianess

[Ruediger Meier]

From: Ruediger Meier <ruediger.meier@ga-group.nl>

The new libfdisk/gpt test (4a4a0927) discovered that we read and write
partition attributes wrongly on BE systems.

Our temporarily used char[8] bits are always LE and do not need to be
converted.

CC: Michael Marineau <michael.marineau@coreos.com>
Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
---
 libfdisk/src/gpt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libfdisk/src/gpt.c b/libfdisk/src/gpt.c
index a7ec539..3db81be 100644
--- a/libfdisk/src/gpt.c
+++ b/libfdisk/src/gpt.c
@@ -1415,7 +1415,7 @@ static int gpt_entry_attrs_to_string(struct gpt_entry *e, char **res)
 	assert(res);
 
 	*res = NULL;
-	attrs = le64_to_cpu(e->attrs);
+	attrs = e->attrs;
 	if (!attrs)
 		return 0;	/* no attributes at all */
 
@@ -1530,7 +1530,7 @@ static int gpt_entry_attrs_from_string(
 			p++;
 	}
 
-	e->attrs = cpu_to_le64(attrs);
+	e->attrs = attrs;
 	return 0;
 }
 
@@ -2517,7 +2517,7 @@ static int gpt_toggle_partition_flag(
 	if ((uint32_t) i >= le32_to_cpu(gpt->pheader->npartition_entries))
 		return -EINVAL;
 
-	attrs = le64_to_cpu(gpt->ents[i].attrs);
+	attrs = gpt->ents[i].attrs;
 	bits = (char *) &attrs;
 
 	switch (flag) {
@@ -2558,7 +2558,7 @@ static int gpt_toggle_partition_flag(
 	else
 		clrbit(bits, bit);
 
-	gpt->ents[i].attrs = cpu_to_le64(attrs);
+	gpt->ents[i].attrs = attrs;
 
 	if (flag == GPT_FLAG_GUIDSPECIFIC)
 		fdisk_info(cxt, isset(bits, bit) ?
-- 
1.8.4.5

Re: [PATCH] libfdisk: (gpt) fix attributes endianess

[Ruediger Meier]

On Friday 08 May 2015, Ruediger Meier wrote:
> From: Ruediger Meier <ruediger.meier@ga-group.nl>
>
> The new libfdisk/gpt test (4a4a0927) discovered that we read and
> write partition attributes wrongly on BE systems.

To be more clear ... this does not mean that 4a4a0927 is broken but the 
test shows that sfdisk (and fdisk) is.

Introduced at least since 01086b80 (2014-03-13) and c77ba531 
(2014-10-02).

Some notes how to reproduce.

## create gpt image
$ truncate -s 10M img
$ ./sfdisk --unit S img <<EOF
label: gpt
size=5M, attrs=""
EOF

## Both commands should do the same
$ ./sfdisk --part-attrs img 1 RequiredPartiton
$ ./test_fdisk_gpt --setattr img 1 "0x0000000000000001"

## So that for both cases and any endianess we should get
$ ./sfdisk --part-attrs img 1
RequiredPartiton
## and
$ ./test_fdisk_gpt --getattr img 1
1: 0x0000000000000001
## on disk it's always little endian
$ hexdump -C img  | grep 009fbe30
009fbe30  01 00 00 00 00 00 00 00  ....


cu,
Rudi

Re: [PATCH] libfdisk: (gpt) fix attributes endianess

[Karel Zak]

On Fri, May 08, 2015 at 01:39:53PM +0200, Ruediger Meier wrote:
> The new libfdisk/gpt test (4a4a0927) discovered that we read and write
> partition attributes wrongly on BE systems.

It was pretty hidden bug (as the problem was in the both set/get
functions).

I have added hexdump to GPT sfdisk test to make sure we produce the
same label on all archs.

Thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] libfdisk: (gpt) fix attributes endianess

[Ruediger Meier]

On Monday 11 May 2015, Karel Zak wrote:
> On Fri, May 08, 2015 at 01:39:53PM +0200, Ruediger Meier wrote:
> > The new libfdisk/gpt test (4a4a0927) discovered that we read and
> > write partition attributes wrongly on BE systems.
>
> It was pretty hidden bug (as the problem was in the both set/get
> functions).

BTW maybe we could add the new raw attributes interface to sfdisk too. 
Like
$ ./sfdisk --part-attrs img 1 "0x0000000000000001"
(and also for stdin text input)

This would make sfdisk forward compatible (in case they invent new 
attributes) and we could remove test_fdisk_gpt helper again.

cu,
Rudi

Re: [PATCH] libfdisk: (gpt) fix attributes endianess

[Karel Zak]

On Tue, May 12, 2015 at 08:47:40AM +0200, Ruediger Meier wrote:
> On Monday 11 May 2015, Karel Zak wrote:
> > On Fri, May 08, 2015 at 01:39:53PM +0200, Ruediger Meier wrote:
> > > The new libfdisk/gpt test (4a4a0927) discovered that we read and
> > > write partition attributes wrongly on BE systems.
> >
> > It was pretty hidden bug (as the problem was in the both set/get
> > functions).
> 
> BTW maybe we could add the new raw attributes interface to sfdisk too. 
> Like
> $ ./sfdisk --part-attrs img 1 "0x0000000000000001"
> (and also for stdin text input)
> 
> This would make sfdisk forward compatible (in case they invent new 
> attributes) and we could remove test_fdisk_gpt helper again.

 Send patch :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] libfdisk: (gpt) fix attributes endianess

[Ruediger Meier]

On Wednesday 13 May 2015, Karel Zak wrote:
> On Tue, May 12, 2015 at 08:47:40AM +0200, Ruediger Meier wrote:
> > On Monday 11 May 2015, Karel Zak wrote:
> > > On Fri, May 08, 2015 at 01:39:53PM +0200, Ruediger Meier wrote:
> > > > The new libfdisk/gpt test (4a4a0927) discovered that we read
> > > > and write partition attributes wrongly on BE systems.
> > >
> > > It was pretty hidden bug (as the problem was in the both set/get
> > > functions).
> >
> > BTW maybe we could add the new raw attributes interface to sfdisk
> > too. Like
> > $ ./sfdisk --part-attrs img 1 "0x0000000000000001"
> > (and also for stdin text input)
> >
> > This would make sfdisk forward compatible (in case they invent new
> > attributes) and we could remove test_fdisk_gpt helper again.
>
>  Send patch :-)

Ok, but still two questions.

1.
Like the example above I'd like to simply interprete hex numbers as 
bitsets. But because of strtol() hex numbers are working already now. 
This does the same:
$ ./sfdisk --part-attrs img 1 "56"
$ ./sfdisk --part-attrs img 1 "0x38"

I guess nobody ever used hexnumbers for GUID so far but the dumps from 
the future sfdisk would behave strange with sfdisk-2.26. So should we 
use a new prefix "bitset:0x..." or "raw:0x..."? 

2.
Rather than suporting whole bitsets only we could simply allow "any bit" 
to let this just work:
$ ./sfdisk --part-attrs img 1 "23"
unsupported GPT attribute bit '23'

cu,
Rudi

Re: [PATCH] libfdisk: (gpt) fix attributes endianess

[Karel Zak]

On Wed, May 13, 2015 at 11:58:05AM +0200, Ruediger Meier wrote:
> On Wednesday 13 May 2015, Karel Zak wrote:
> > On Tue, May 12, 2015 at 08:47:40AM +0200, Ruediger Meier wrote:
> > > On Monday 11 May 2015, Karel Zak wrote:
> > > > On Fri, May 08, 2015 at 01:39:53PM +0200, Ruediger Meier wrote:
> > > > > The new libfdisk/gpt test (4a4a0927) discovered that we read
> > > > > and write partition attributes wrongly on BE systems.
> > > >
> > > > It was pretty hidden bug (as the problem was in the both set/get
> > > > functions).
> > >
> > > BTW maybe we could add the new raw attributes interface to sfdisk
> > > too. Like
> > > $ ./sfdisk --part-attrs img 1 "0x0000000000000001"
> > > (and also for stdin text input)
> > >
> > > This would make sfdisk forward compatible (in case they invent new
> > > attributes) and we could remove test_fdisk_gpt helper again.
> >
> >  Send patch :-)
> 
> Ok, but still two questions.
> 
> 1.
> Like the example above I'd like to simply interprete hex numbers as 
> bitsets. But because of strtol() hex numbers are working already now. 
> This does the same:
> $ ./sfdisk --part-attrs img 1 "56"
> $ ./sfdisk --part-attrs img 1 "0x38"
> 
> I guess nobody ever used hexnumbers for GUID so far but the dumps from 
> the future sfdisk would behave strange with sfdisk-2.26. So should we 
> use a new prefix "bitset:0x..." or "raw:0x..."? 

We nowhere use hex numbers now (and frankly --part-attrs img "0x38" is
side-effect of strtol() rather than planned behavior :-) and for dumps
and another outputs we use gpt_entry_attrs_to_string() where is also no
hex numbers, but list of bits.

IMHO it would be better to detect 0x prefix in gpt_entry_attrs_from_string() 
and if specified than use the number as new attribute rather than toggle 
individual bits. 

And mix normal numbers and hex number should be forbidden. It means
that the string for gpt_entry_attrs_from_string() should be:

 <ATTR-STR> : <HEX>|<LIST>

 <LIST> : <NAME>|<NUM> [, ...]
 <HEX>  : "0x"

where <LIST> toggle individual bits, and <HEX> replaces all attribute.

In all cases we need to verify that final attribute number is
compatible with EFI standard (see below). It would be probably good to
add gpt_verify_attrs() and use it also in
fdisk_gpt_set_partition_attrs().

> 2.
> Rather than suporting whole bitsets only we could simply allow "any bit" 
> to let this just work:
> $ ./sfdisk --part-attrs img 1 "23"
> unsupported GPT attribute bit '23'

That's question, EFI standard specifies that only bits in range 48..64
are "free" and maybe used for arbitrary purpose. The rest is owned by
standard (and currently standard uses first three bits only). So I
think it's fine that you cannot toggle bit "23".

The code in gpt_entry_attrs_from_string() allows to address by numbers
only the "free" bits. This is probably too restrictive, it should be
also possible to address EFI standard bits (RequiredPartiton,
NoBlockIOProtocol and LegacyBIOSBootable) by numbers.

The new API fdisk_gpt_set_partition_attrs() seems pretty open and
allows to set arbitrary bits, maybe it should be more restrictive to
keep unused standard bits unmodified.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com