[PATCH] daemon/mkfs: disable creation of fake MBR partition table with "mkfs.fat"
Search the usage output of "mkfs.fat" for "--mbr[="; cache the result for further invocations. If the option is supported, pass "--mbr=n" to "mkfs.fat". This will prevent the creation of a bogus partition table whose first (and only) entry describes a partition that contains the partition table. (Such a bogus partition table breaks "parted", which is a tool used by libguestfs extensively, both internally and in public libguestfs APIs.) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Testing (per <https://bugzilla.redhat.com/show_bug.cgi?id=1931821#c0>): $ virt-make-fs -vx --format=raw --size=200M --type=vfat --label=TEST \ website ~/test_vfat.img > [...] > creating vfat filesystem on /dev/sda ... > libguestfs: trace: mkfs "vfat" "/dev/sda" "label:TEST" > [...] > commandrvf: stdout=n stderr=y flags=0x0 > commandrvf: mkfs.fat > Usage: mkfs.fat [OPTIONS] TARGET [BLOCKS] > Create FAT filesystem in TARGET, which can be a block device or file. Use only > up to BLOCKS 1024 byte blocks if specified. With the -C option, file TARGET will be > created with a size of 1024 bytes times BLOCKS, which must be specified. > > Options: > -a Disable alignment of data structures > -A Toggle Atari variant of the filesystem > -b SECTOR Select SECTOR as location of the FAT32 backup boot sector > -c Check device for bad blocks before creating the filesystem > -C Create file TARGET then create filesystem in it > -D NUMBER Write BIOS drive number NUMBER to boot sector > -f COUNT Create COUNT file allocation tables > -F SIZE Select FAT size SIZE (12, 16 or 32) > -g GEOM Select disk geometry: heads/sectors_per_track > -h NUMBER Write hidden sectors NUMBER to boot sector > -i VOLID Set volume ID to VOLID (a 32 bit hexadecimal number) > -I Ignore and disable safety checks > -l FILENAME Read bad blocks list from FILENAME > -m FILENAME Replace default error message in boot block with contents of FILENAME > -M TYPE Set media type in boot sector to TYPE > --mbr[=y|n|a] Fill (fake) MBR table with one partition which spans whole disk > -n LABEL Set volume name to LABEL (up to 11 characters long) > --codepage=N use DOS codepage N to encode label (default: 850) > -r COUNT Make room for at least COUNT entries in the root directory > -R COUNT Set minimal number of reserved sectors to COUNT > -s COUNT Set number of sectors per cluster to COUNT > -S SIZE Select a sector size of SIZE (a power of two, at least 512) > -v Verbose execution > --variant=TYPE Select variant TYPE of filesystem (standard or Atari) > > --invariant Use constants for randomly generated or time based values > --offset=SECTOR Write the filesystem at a specific sector into the device file. > --help Show this help message and exit > [...] > commandrvf: stdout=n stderr=y flags=0x0 > commandrvf: mkfs -t vfat -I --mbr=n -n TEST /dev/sda $ guestfish -a ~/test_vfat.img > ><fs> run > ><fs> list-filesystems > /dev/sda: vfat daemon/mkfs.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/daemon/mkfs.c b/daemon/mkfs.c index ba90cef2111c..4cb216cabe8e 100644 --- a/daemon/mkfs.c +++ b/daemon/mkfs.c @@ -30,6 +30,14 @@ #define MAX_ARGS 64 +enum fat_mbr_option { + FMO_UNCHECKED, + FMO_DOESNT_EXIST, + FMO_EXISTS, +}; + +static enum fat_mbr_option fat_mbr_option = FMO_UNCHECKED; + /* Takes optional arguments, consult optargs_bitmask. */ int do_mkfs (const char *fstype, const char *device, int blocksize, @@ -101,6 +109,25 @@ do_mkfs (const char *fstype, const char *device, int blocksize, STREQ (fstype, "msdos")) ADD_ARG (argv, i, "-I"); + /* Prevent mkfs.fat from creating a bogus partition table (RHBZ#1931821). */ + if (STREQ (fstype, "fat") || STREQ (fstype, "vfat") || + STREQ (fstype, "msdos")) { + if (fat_mbr_option == FMO_UNCHECKED) { + CLEANUP_FREE char *usage_err = NULL; + + fat_mbr_option = FMO_DOESNT_EXIST; + /* Invoking either version 3 of version 4 of mkfs.fat without any options + * will make it (a) print a usage summary to stderr, (b) exit with status + * 1. + */ + r = commandr (NULL, &usage_err, "mkfs.fat", (char *)NULL); + if (r == 1 && strstr (usage_err, "--mbr[=") != NULL) + fat_mbr_option = FMO_EXISTS; + } + if (fat_mbr_option == FMO_EXISTS) + ADD_ARG (argv, i, "--mbr=n"); + } + /* Process blocksize parameter if set. */ if (optargs_bitmask & GUESTFS_MKFS_BLOCKSIZE_BITMASK) { if (blocksize <= 0 || !is_power_of_2 (blocksize)) { -- 2.19.1.3.g30247aa5d201
On Tue, Nov 23, 2021 at 03:00:50PM +0100, Laszlo Ersek wrote:
Search the usage output of "mkfs.fat" for "--mbr[="; cache the result for further invocations. If the option is supported, pass "--mbr=n" to "mkfs.fat". This will prevent the creation of a bogus partition table whose first (and only) entry describes a partition that contains the partition table.
(Such a bogus partition table breaks "parted", which is a tool used by libguestfs extensively, both internally and in public libguestfs APIs.)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1931821 Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: Testing (per <https://bugzilla.redhat.com/show_bug.cgi?id=1931821#c0>):
$ virt-make-fs -vx --format=raw --size=200M --type=vfat --label=TEST \ website ~/test_vfat.img
> [...] > creating vfat filesystem on /dev/sda ... > libguestfs: trace: mkfs "vfat" "/dev/sda" "label:TEST" > [...] > commandrvf: stdout=n stderr=y flags=0x0 > commandrvf: mkfs.fat > Usage: mkfs.fat [OPTIONS] TARGET [BLOCKS] > Create FAT filesystem in TARGET, which can be a block device or file. Use only > up to BLOCKS 1024 byte blocks if specified. With the -C option, file TARGET will be > created with a size of 1024 bytes times BLOCKS, which must be specified. > > Options: > -a Disable alignment of data structures > -A Toggle Atari variant of the filesystem > -b SECTOR Select SECTOR as location of the FAT32 backup boot sector > -c Check device for bad blocks before creating the filesystem > -C Create file TARGET then create filesystem in it > -D NUMBER Write BIOS drive number NUMBER to boot sector > -f COUNT Create COUNT file allocation tables > -F SIZE Select FAT size SIZE (12, 16 or 32) > -g GEOM Select disk geometry: heads/sectors_per_track > -h NUMBER Write hidden sectors NUMBER to boot sector > -i VOLID Set volume ID to VOLID (a 32 bit hexadecimal number) > -I Ignore and disable safety checks > -l FILENAME Read bad blocks list from FILENAME > -m FILENAME Replace default error message in boot block with contents of FILENAME > -M TYPE Set media type in boot sector to TYPE > --mbr[=y|n|a] Fill (fake) MBR table with one partition which spans whole disk > -n LABEL Set volume name to LABEL (up to 11 characters long) > --codepage=N use DOS codepage N to encode label (default: 850) > -r COUNT Make room for at least COUNT entries in the root directory > -R COUNT Set minimal number of reserved sectors to COUNT > -s COUNT Set number of sectors per cluster to COUNT > -S SIZE Select a sector size of SIZE (a power of two, at least 512) > -v Verbose execution > --variant=TYPE Select variant TYPE of filesystem (standard or Atari) > > --invariant Use constants for randomly generated or time based values > --offset=SECTOR Write the filesystem at a specific sector into the device file. > --help Show this help message and exit > [...] > commandrvf: stdout=n stderr=y flags=0x0 > commandrvf: mkfs -t vfat -I --mbr=n -n TEST /dev/sda
$ guestfish -a ~/test_vfat.img
> ><fs> run > ><fs> list-filesystems > /dev/sda: vfat
daemon/mkfs.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/daemon/mkfs.c b/daemon/mkfs.c index ba90cef2111c..4cb216cabe8e 100644 --- a/daemon/mkfs.c +++ b/daemon/mkfs.c @@ -30,6 +30,14 @@
#define MAX_ARGS 64
+enum fat_mbr_option { + FMO_UNCHECKED, + FMO_DOESNT_EXIST, + FMO_EXISTS, +}; + +static enum fat_mbr_option fat_mbr_option = FMO_UNCHECKED; + /* Takes optional arguments, consult optargs_bitmask. */ int do_mkfs (const char *fstype, const char *device, int blocksize, @@ -101,6 +109,25 @@ do_mkfs (const char *fstype, const char *device, int blocksize, STREQ (fstype, "msdos")) ADD_ARG (argv, i, "-I");
+ /* Prevent mkfs.fat from creating a bogus partition table (RHBZ#1931821). */ + if (STREQ (fstype, "fat") || STREQ (fstype, "vfat") || + STREQ (fstype, "msdos")) { + if (fat_mbr_option == FMO_UNCHECKED) { + CLEANUP_FREE char *usage_err = NULL; + + fat_mbr_option = FMO_DOESNT_EXIST; + /* Invoking either version 3 of version 4 of mkfs.fat without any options + * will make it (a) print a usage summary to stderr, (b) exit with status + * 1. + */ + r = commandr (NULL, &usage_err, "mkfs.fat", (char *)NULL); + if (r == 1 && strstr (usage_err, "--mbr[=") != NULL) + fat_mbr_option = FMO_EXISTS; + } + if (fat_mbr_option == FMO_EXISTS) + ADD_ARG (argv, i, "--mbr=n"); + } + /* Process blocksize parameter if set. */ if (optargs_bitmask & GUESTFS_MKFS_BLOCKSIZE_BITMASK) { if (blocksize <= 0 || !is_power_of_2 (blocksize)) {
Yeah that looks like the right fix to me. Acked-by: Richard W.M. Jones <rjones@redhat.com> A side note, in case it wasn't obvious already: The daemon is single threaded, so there's no need to lock access to that static variable. We've discussed many times making the daemon be multi-threaded (eg. to support a single library instance making several connections into the daemon to support parallel operations) but it's such a substantial change to the way that libguestfs works we've never done it, and aren't likely to any time soon. 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
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones