Re: PR toolchain/42357 disklabel endian issues

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

lloyd%must-have-coffee.gen.nz@localhost wrote:

> I have submitted the above PR with a bunch of patches for fixing
> this problem. Some review would be nice. 

Ok, I have some comments.

> This adds a function called bswap_label() that byte swaps a label.

IMO, such byte swap ops should be done more carefully.
In general, it is not a good idea to make the same variable
have both endianness because it makes harder for readers
to know which endianness each variable has at any point.

In your patch, BSWAP_LABEL() is applied to struct disklabel *disk_lp,
which points to label_sector buffer. The label_sector contains
"raw" data for the target disk and it should always be target endian
otherwise you might write data in wrong endian to target image.

For example, write_bootarea() is called right after find_label()
if -D is specfied and in that case target disk will have a wrong
endian label (though disklabel is invalidated by -D anyway).

In my patch, label_sector always has disklabel in target endian
and all byte swap ops are done when it's copied between
struct disklabel variable for the host and target buffer.
It means struct disklabel *lp has always host endian and
struct disklabel *disk_lp (which is used for label_sector)
always has target endian.

> The dkcksum function has been changed to take the number of
> partitions in the label as a parameter because it sometimes needs
> to checksum a byte swapped label. I could have changed dkcksum to
> check the label magic numbers and byte swap the npartitions field
> if necessary, but that would have introduced a lot of new
> dependencies to dkcksum, so I opted for the extra parameter.

I don't think it's a good idea to change an existing exported API.
Current dkcksum() interface assumes that passed struct disklabel
has valid MAGICs and a sane number of npartitions, and there is
a similar dkcksum() function in src/sys/subr_disk.c.

It's better to keep existing API and create a new function,
as current sys/kern/subr_disk.c so that you don't have to touch
sources other than ones in disklabel(8).

We should always know which endian struct disklabel passed to
dkcksum() (or similar API) is in any case, and if we are passing
struct disklabel for swapped target ones, we should use a different
API which doesn't assume npartitions is not in host endian.

> I also replaced a couple of u_short with uint16_t because we are
> dealing with data types whose size is fixed externally. (u_short
> may be defined to have a fixed size, but there's no harm in
> saying what you mean.)

Well, I have already committed this part, because this is
independent issue from endianness.

> @@ -933,15 +940,12 @@
>                       is_deleted = "deleted ";
>               }
>               if (lp->d_magic != DISKMAGIC) {
> -                     /* XXX: Do something about byte-swapped labels ? */
>                       if (lp->d_magic == DISKMAGIC_REV &&
>                           lp->d_magic2 == DISKMAGIC_REV)
> -                             warnx("ignoring %sbyteswapped label"
> -                                 " at offset %u from sector %u",
> -                                 is_deleted, offset, sector);
> -                     continue;
> +                             bswap_label (lp);
>               }

This is wrong. This should check if disklabel in label_sector
(i.e. in raw disk image) has the same endianness with the "target".
We should not swap endianness as a result of comparison of image
and host. Instead, we should check like
"targettohost32(lp->d_magic) == DISKMAGIC_REV"
as my patch does.

> +     if (l->d_magic == DISKMAGIC && l->d_magic2 == DISKMAGIC)
> +             npartitions = l->d_npartitions;
> +     else if (l->d_magic == DISKMAGIC_REV && l->d_magic2 == DISKMAGIC_REV)
> +             npartitions = bswap16 (l->d_npartitions);
> +     else
> +             return;
> +
> +     /* We need to make sure that we don't replace a bad checksum with a 
> +      * good one, but we don't need to tell anyone because we are preserving
> +      * the bad checksum for others to check.
> +      */
> +     if (dkcksum(l, npartitions) != 0)
> +             return;

IMO, even if magic and npartitions are not sane, we should
assume npartition is zero and swap members anyway.

> +     l->d_magic = bswap32 (l->d_magic);
 :

You leave d_spare[NSPARE] members, but I think we should
swap them even if they are unused, so that we can search
all usage of the member by name when the reserved member
is renamed and actually used in future.

---
Izumi Tsutsui