thr3ads.net - Libguestfs - [Libguestfs] [PATCH v3 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-25 09:49 UTC

[Libguestfs] [PATCH v3 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>
Acked-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v3:
    
    - pick up Rich's ACK
    
    - drop "Unix." namespace selector from "read" [Rich]
    
    - factor "Bytes.get_uint8 sec0" out to "sec0at" [Rich,
Laszlo]
    
    v2:
    
    - new patch (the hard one)

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

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. *)
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..c51a0d181365 100644
--- a/daemon/utils.ml
+++ b/daemon/utils.ml
@@ -187,6 +187,51 @@ 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 = read fd sec0 0 sec0size in
+      let sec0at = fun offset -> Bytes.get_uint8 sec0 offset in
+
+      (* sector read completely *)
+      sec0read = sec0size &&
+
+      (* boot signature present *)
+      sec0at (sigofs       ) = 0x55 &&
+      sec0at (sigofs  + 0x1) = 0xAA &&
+
+      (* mkfs.fat signature present *)
+      Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat"
&&
+
+      (* partition bootable *)
+      sec0at (pte1ofs      ) = 0x80 &&
+
+      (* partition starts at C/H/S 0/0/1 *)
+      sec0at (pte1ofs + 0x1) = 0x00 &&
+      sec0at (pte1ofs + 0x2) = 0x01 &&
+      sec0at (pte1ofs + 0x3) = 0x00 &&
+
+      (* partition type is a FAT variant that mkfs.fat is known to create *)
+      List.mem (sec0at (pte1ofs + 0x4)) parttypes &&
+
+      (* partition starts at LBA 0 *)
+      sec0at (pte1ofs + 0x8) = 0x00 &&
+      sec0at (pte1ofs + 0x9) = 0x00 &&
+      sec0at (pte1ofs + 0xA) = 0x00 &&
+      sec0at (pte1ofs + 0xB) = 0x00
+    )
+  with _ -> false
+
 let proc_unmangle_path path    let n = String.length path in
   let b = Buffer.create n in
-- 
2.19.1.3.g30247aa5d201

Richard W.M. Jones

2021-Nov-25 10:15 UTC

head link

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

On Thu, Nov 25, 2021 at 10:49:53AM +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>
> Acked-by: Richard W.M. Jones <rjones at redhat.com>
> ---
> 
> Notes:
>     v3:
>     
>     - pick up Rich's ACK
>     
>     - drop "Unix." namespace selector from "read"
[Rich]
>     
>     - factor "Bytes.get_uint8 sec0" out to "sec0at"
[Rich, Laszlo]
>     
>     v2:
>     
>     - new patch (the hard one)
> 
>  daemon/utils.mli | 10 ++++++++++
>  daemon/parted.ml |  7 ++++++-
>  daemon/utils.ml  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 1 deletion(-)
> 
> 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. *)
> 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..c51a0d181365 100644
> --- a/daemon/utils.ml
> +++ b/daemon/utils.ml
> @@ -187,6 +187,51 @@ 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 = read fd sec0 0 sec0size in
> +      let sec0at = fun offset -> Bytes.get_uint8 sec0 offset in
I think this should be:

  let sec0at = Bytes.get_uint8 sec0 in

Otherwise the patch series looks fine to me.

Rich.
> +      (* sector read completely *)
> +      sec0read = sec0size &&
> +
> +      (* boot signature present *)
> +      sec0at (sigofs       ) = 0x55 &&
> +      sec0at (sigofs  + 0x1) = 0xAA &&
> +
> +      (* mkfs.fat signature present *)
> +      Bytes.sub_string sec0 sysidofs sysidsize = "mkfs.fat"
&&
> +
> +      (* partition bootable *)
> +      sec0at (pte1ofs      ) = 0x80 &&
> +
> +      (* partition starts at C/H/S 0/0/1 *)
> +      sec0at (pte1ofs + 0x1) = 0x00 &&
> +      sec0at (pte1ofs + 0x2) = 0x01 &&
> +      sec0at (pte1ofs + 0x3) = 0x00 &&
> +
> +      (* partition type is a FAT variant that mkfs.fat is known to create
*)
> +      List.mem (sec0at (pte1ofs + 0x4)) parttypes &&
> +
> +      (* partition starts at LBA 0 *)
> +      sec0at (pte1ofs + 0x8) = 0x00 &&
> +      sec0at (pte1ofs + 0x9) = 0x00 &&
> +      sec0at (pte1ofs + 0xA) = 0x00 &&
> +      sec0at (pte1ofs + 0xB) = 0x00
> +    )
> +  with _ -> false
> +
>  let proc_unmangle_path path >    let n = String.length path in
>    let b = Buffer.create n in
> -- 
> 2.19.1.3.g30247aa5d201
> 
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
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

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

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

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