thr3ads.net - Libguestfs - [Libguestfs] [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted" [Nov 2021]

If this information is useful, please help other people find it:
Share via:

Laszlo Ersek

2021-Nov-24 10:37 UTC

[Libguestfs] [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted"

"parted" incorrectly reports "loop" rather than
"msdos" for the partition
table type, when the (fake) partition table comes from the "--mbr"
option
of "mkfs.fat" (in dosfstools-4.2+), and the FAT variant in question is
FAT16 or FAT32. (See RHBZ#2026224.) Work this around by
- parsing the partition table ourselves, and
- overriding "loop" with "msdos" when appropriate.

Note that when the FAT variant is FAT12, "parted" fails to parse the
fake
MBR partition table completely (see RHBZ#2026220), which we cannot work
around. However, FAT12 should be a rare corner case in libguestfs usage --
"mkfs.fat" auto-chooses FAT12 only below 9MB disk size, and even
"-F 12"
can only be forced up to and including 255MB disk size.

Add the helper function "has_bogus_mbr" to the Utils module; we'll
use it
elsewhere too.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v2:
    
    - new patch (the hard one)

 daemon/parted.ml |  7 ++++++-
 daemon/utils.ml  | 44 ++++++++++++++++++++++++++++++++++++++++++++
 daemon/utils.mli | 10 ++++++++++
 3 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/daemon/parted.ml b/daemon/parted.ml
index 29b98c4806c1..53077d91afdd 100644
--- a/daemon/parted.ml
+++ b/daemon/parted.ml
@@ -118,7 +118,12 @@ let part_get_parttype device    let fields = String.nsplit
":" device_line in
   match fields with
   | _::_::_::_::_::"loop"::_ -> (* If "loop" return an
error (RHBZ#634246). *)
-     failwithf "%s: not a partitioned device" device
+     (* ... Unless parted failed to recognize the fake MBR that mkfs.fat from
+      * dosfstools-4.2+ created. In that case, return "msdos" for MBR
+      * (RHBZ#1931821).
+      *)
+     if Utils.has_bogus_mbr device then "msdos"
+     else failwithf "%s: not a partitioned device" device
   | _::_::_::_::_::ret::_ -> ret
   | _ ->
      failwithf "%s: cannot parse the output of parted" device
diff --git a/daemon/utils.ml b/daemon/utils.ml
index b56306055634..7ef3c206b71a 100644
--- a/daemon/utils.ml
+++ b/daemon/utils.ml
@@ -187,6 +187,50 @@ and compare_device_names a b      )
   )
 
+let has_bogus_mbr device +  try
+    with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd ->
+      let sec0size = 0x200
+      and sigofs = 0x1FE
+      and sysidofs = 0x003 and sysidsize = 0x008
+      and pte1ofs = 0x1BE
+      and parttypes = [0x01; (* FAT12 *)
+                       0x04; (* FAT16 *)
+                       0x06; (* FAT12, FAT16, FAT16B *)
+                       0x0C; (* FAT32 LBA *)
+                       0x0E  (* FAT16B LBA *)] in
+      let sec0 = Bytes.create sec0size in
+      let sec0read = Unix.read fd sec0 0 sec0size in
+
+      (* sector read completely *)
+      sec0read = sec0size &&
+
+      (* boot signature present *)
+      Bytes.get_uint8 sec0 (sigofs       ) = 0x55 &&
+      Bytes.get_uint8 sec0 (sigofs  + 0x1) = 0xAA &&
+
+      (* mkfs.fat signature present *)
+      Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat"
&&
+
+      (* partition bootable *)
+      Bytes.get_uint8 sec0 (pte1ofs      ) = 0x80 &&
+
+      (* partition starts at C/H/S 0/0/1 *)
+      Bytes.get_uint8 sec0 (pte1ofs + 0x1) = 0x00 &&
+      Bytes.get_uint8 sec0 (pte1ofs + 0x2) = 0x01 &&
+      Bytes.get_uint8 sec0 (pte1ofs + 0x3) = 0x00 &&
+
+      (* partition type is a FAT variant that mkfs.fat is known to create *)
+      List.mem (Bytes.get_uint8 sec0 (pte1ofs + 0x4)) parttypes &&
+
+      (* partition starts at LBA 0 *)
+      Bytes.get_uint8 sec0 (pte1ofs + 0x8) = 0x00 &&
+      Bytes.get_uint8 sec0 (pte1ofs + 0x9) = 0x00 &&
+      Bytes.get_uint8 sec0 (pte1ofs + 0xA) = 0x00 &&
+      Bytes.get_uint8 sec0 (pte1ofs + 0xB) = 0x00
+    )
+  with _ -> false
+
 let proc_unmangle_path path    let n = String.length path in
   let b = Buffer.create n in
diff --git a/daemon/utils.mli b/daemon/utils.mli
index ec1127f7211e..d7b7aeba0657 100644
--- a/daemon/utils.mli
+++ b/daemon/utils.mli
@@ -56,6 +56,16 @@ val sort_device_names : string list -> string list
     This also deals with partition numbers, and works whether or not
     [/dev/] is present. *)
 
+val has_bogus_mbr : string -> bool
+(** Check whether the first sector of the device contains a bogus MBR partition
+    table; namely one where the first partition table entry describes a
+    partition that starts at absolute sector 0, thereby overlapping the
+    partition table itself.
+
+    dosfstools-4.2+ creates bogus partition tables like this by default when
+    formatting non-removable, non-partitioned block devices. Refer to
+    RHBZ#1931821. *)
+
 val proc_unmangle_path : string -> string
 (** Reverse kernel path escaping done in fs/seq_file.c:mangle_path.
     This is inconsistently used for /proc fields. *)
-- 
2.19.1.3.g30247aa5d201

Richard W.M. Jones

2021-Nov-24 16:45 UTC

head link

[Libguestfs] [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted"

On Wed, Nov 24, 2021 at 11:37:46AM +0100, Laszlo Ersek
wrote:> "parted" incorrectly reports "loop" rather than
"msdos" for the partition
> table type, when the (fake) partition table comes from the
"--mbr" option
> of "mkfs.fat" (in dosfstools-4.2+), and the FAT variant in
question is
> FAT16 or FAT32. (See RHBZ#2026224.) Work this around by
> - parsing the partition table ourselves, and
> - overriding "loop" with "msdos" when appropriate.
> 
> Note that when the FAT variant is FAT12, "parted" fails to parse
the fake
> MBR partition table completely (see RHBZ#2026220), which we cannot work
> around. However, FAT12 should be a rare corner case in libguestfs usage --
> "mkfs.fat" auto-chooses FAT12 only below 9MB disk size, and even
"-F 12"
> can only be forced up to and including 255MB disk size.
> 
> Add the helper function "has_bogus_mbr" to the Utils module;
we'll use it
> elsewhere too.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
> 
> Notes:
>     v2:
>     
>     - new patch (the hard one)
> 
>  daemon/parted.ml |  7 ++++++-
>  daemon/utils.ml  | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  daemon/utils.mli | 10 ++++++++++
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/parted.ml b/daemon/parted.ml
> index 29b98c4806c1..53077d91afdd 100644
> --- a/daemon/parted.ml
> +++ b/daemon/parted.ml
> @@ -118,7 +118,12 @@ let part_get_parttype device >    let fields =
String.nsplit ":" device_line in
>    match fields with
>    | _::_::_::_::_::"loop"::_ -> (* If "loop" return
an error (RHBZ#634246). *)
> -     failwithf "%s: not a partitioned device" device
> +     (* ... Unless parted failed to recognize the fake MBR that mkfs.fat
from
> +      * dosfstools-4.2+ created. In that case, return "msdos"
for MBR
> +      * (RHBZ#1931821).
> +      *)
> +     if Utils.has_bogus_mbr device then "msdos"
> +     else failwithf "%s: not a partitioned device" device
>    | _::_::_::_::_::ret::_ -> ret
>    | _ ->
>       failwithf "%s: cannot parse the output of parted" device
> diff --git a/daemon/utils.ml b/daemon/utils.ml
> index b56306055634..7ef3c206b71a 100644
> --- a/daemon/utils.ml
> +++ b/daemon/utils.ml
> @@ -187,6 +187,50 @@ and compare_device_names a b >      )
>    )
>  
> +let has_bogus_mbr device > +  try
> +    with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd ->
> +      let sec0size = 0x200
> +      and sigofs = 0x1FE
> +      and sysidofs = 0x003 and sysidsize = 0x008
> +      and pte1ofs = 0x1BE
> +      and parttypes = [0x01; (* FAT12 *)
> +                       0x04; (* FAT16 *)
> +                       0x06; (* FAT12, FAT16, FAT16B *)
> +                       0x0C; (* FAT32 LBA *)
> +                       0x0E  (* FAT16B LBA *)] in
> +      let sec0 = Bytes.create sec0size in
> +      let sec0read = Unix.read fd sec0 0 sec0size in
> +
> +      (* sector read completely *)
> +      sec0read = sec0size &&
> +
> +      (* boot signature present *)
> +      Bytes.get_uint8 sec0 (sigofs       ) = 0x55 &&
> +      Bytes.get_uint8 sec0 (sigofs  + 0x1) = 0xAA &&
> +
> +      (* mkfs.fat signature present *)
> +      Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat"
&&
> +
> +      (* partition bootable *)
> +      Bytes.get_uint8 sec0 (pte1ofs      ) = 0x80 &&
> +
> +      (* partition starts at C/H/S 0/0/1 *)
> +      Bytes.get_uint8 sec0 (pte1ofs + 0x1) = 0x00 &&
> +      Bytes.get_uint8 sec0 (pte1ofs + 0x2) = 0x01 &&
> +      Bytes.get_uint8 sec0 (pte1ofs + 0x3) = 0x00 &&
> +
> +      (* partition type is a FAT variant that mkfs.fat is known to create
*)
> +      List.mem (Bytes.get_uint8 sec0 (pte1ofs + 0x4)) parttypes &&
> +
> +      (* partition starts at LBA 0 *)
> +      Bytes.get_uint8 sec0 (pte1ofs + 0x8) = 0x00 &&
> +      Bytes.get_uint8 sec0 (pte1ofs + 0x9) = 0x00 &&
> +      Bytes.get_uint8 sec0 (pte1ofs + 0xA) = 0x00 &&
> +      Bytes.get_uint8 sec0 (pte1ofs + 0xB) = 0x00
> +    )
> +  with _ -> false
It's not wrong, but you might consider factoring out some of those
common functions.

First of all, take a look at this code for inspiration since it's
doing something which is a bit similar:
https://github.com/libguestfs/libguestfs/blob/e7f72ab146b9c2aaee92a600a1fcbefb0202d41c/daemon/isoinfo.ml#L82

Using factoring you could write it as:

  let is offs b = Bytes.get_uint8 sec0 offs = b in
  is  sigofs        0x55 &&
  is (sigofs + 0x1) 0xaa && ...

It's a matter of taste though, not a blocker.

Looks good, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Richard W.M. Jones

2021-Nov-24 16:47 UTC

head link

[Libguestfs] [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted"

On Wed, Nov 24, 2021 at 11:37:46AM +0100, Laszlo Ersek
wrote:> +let has_bogus_mbr device > +  try
> +    with_openfile device [O_RDONLY; O_CLOEXEC] 0 (fun fd ->
> +      let sec0size = 0x200
> +      and sigofs = 0x1FE
> +      and sysidofs = 0x003 and sysidsize = 0x008
> +      and pte1ofs = 0x1BE
> +      and parttypes = [0x01; (* FAT12 *)
> +                       0x04; (* FAT16 *)
> +                       0x06; (* FAT12, FAT16, FAT16B *)
> +                       0x0C; (* FAT32 LBA *)
> +                       0x0E  (* FAT16B LBA *)] in
> +      let sec0 = Bytes.create sec0size in
> +      let sec0read = Unix.read fd sec0 0 sec0size in
I forgot to say that since Unix has been "open"-d here, you don't
need
the "Unix." prefix, unless you think it's necessary for clarity
about
which particular read function is being called.  (I personally
wouldn't use it because it's like the read(2) function in C which also
doesn't have a prefix.)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Libguestfs - Nov 2021 - [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted"

[Libguestfs] [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted"

[Libguestfs] [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted"

[Libguestfs] [PATCH v2 4/5] daemon/parted: work around part table type misreporting by "parted"