[parted-devel] [PATCH] libparted: reallocate buf after _disk_analyse_block_size call

[Jim Meyering]

Jim Meyering wrote:

> Jim Meyering wrote:
> ...
>> Thanks for the added detail.
>> Would you please include something like that to the commit log
>> for your patch?
>>
>> Re testing, would it be enough to use parted to create a MAC partition
>> table with "mklabel mac", add at least one partition, and then use dd or
>
> BTW, I realized that we can probably reproduce it with no partition at all.
>
>> perl to poke a new, larger number into its 16-bit big-endian
>> raw_disk->block_size field ?
>
> I've gone ahead and adjusted your patch.
> Can you confirm that this still solves your problem
> and sign off on the modifications?
> I've also removed an unnecessary cast.
>
> I'll look at creating a test case tomorrow.
>
...
> +	MacRawDisk *raw_disk = buf;
>
>  	if (!_check_signature (raw_disk))
>  		goto error;
>
> +	/* Record the original sector size;  this function may change it.  */
> +	PedSector ss0 = disk->dev->sector_size;
>  	if (!_disk_analyse_block_size (disk, raw_disk))
>  		goto error;
> +	/* If _disk_analyse_block_size has increased the sector_size,
> +	   reallocate this buffer, so we can still read a sector into it.  */
> +	if (ss0 < disk->dev->sector_size) {
> +		free (buf);
> +		buf = ped_malloc (disk->dev->sector_size);
> +		if (buf == NULL)
> +			goto error;
> +	}

In testing the above, I realized that the way I changed your
patch (in hoisting the reallocation) was invalid, since there
are uses of raw_disk (aliased to "buf") after the point where
my hoisted code freed buf.  This one works better:

FTR, here's the basis for the test I'm using:

  dev=dev-file
  dd if=/dev/null of=$dev bs=1 seek=2MB
  parted -s -- $dev mklabel mac
  # Poke a big-endian 1024 into the 2-byte block_size slot.
  perl -e 'print pack("S>", 1024)'|dd of=$dev bs=1 seek=2 count=2 conv=notrunc
  parted $dev u s p
  # Respond "i" (for ignore) to the first prompt.
  # That would trigger the buffer overrun.


>From f77b6b002c92f62c8c0e8d502404bfd94983a81e Mon Sep 17 00:00:00 2001
From: "Brian C. Lane" <bcl at redhat.com>
Date: Tue, 4 Sep 2012 16:42:34 -0700
Subject: [PATCH] mac: don't let larger partition-specified block size evoke
 UB

For example, in reading a MAC partition table on a 512-byte sector-size
disk, _disk_analyse_block_size could find reason to ask if it's ok to
increase that to e.g., 2048.  Upon a positive reply, we would read 2048
bytes into a 512-byte buffer.

* libparted/labels/mac.c (mac_read): If needed, reallocate "buf"
to accommodate a new, larger sector size.
* NEWS (Bug fixes): Mention it.
---
 NEWS                   |  7 +++++++
 libparted/labels/mac.c | 14 +++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index f929b99..bab3afb 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,13 @@ GNU parted NEWS                                    -*- outline -*-

 ** Bug Fixes

+  libparted: mac: a MAC partition table could have a block_size larger
+  than the one the kernel told us about.  Upon reading that partition
+  table, libparted would ask if it's ok to use the larger block size.
+  If you were to respond in the affirmative, libparted would read the
+  larger number of bytes into a buffer of the shorter length,
+  overrunning it.
+
   libparted: gpt: fix gpt_get_max_supported_partition_count to work
   also on little-endian systems.

diff --git a/libparted/labels/mac.c b/libparted/labels/mac.c
index 1f59a1a..2485187 100644
--- a/libparted/labels/mac.c
+++ b/libparted/labels/mac.c
@@ -738,13 +738,16 @@ mac_read (PedDisk* disk)
 	if (!ptt_read_sector (disk->dev, 0, &buf))
 		return 0;

-	MacRawDisk *raw_disk = (MacRawDisk *) buf;
+	MacRawDisk *raw_disk = buf;

 	if (!_check_signature (raw_disk))
 		goto error;

+	/* Record the original sector size;  this function may change it.  */
+	PedSector ss0 = disk->dev->sector_size;
 	if (!_disk_analyse_block_size (disk, raw_disk))
 		goto error;
+
 	if (!_disk_analyse_ghost_size (disk))
 		goto error;
 	ghost_size = mac_disk_data->ghost_size;
@@ -759,6 +762,15 @@ mac_read (PedDisk* disk)
 		mac_disk_data->block_size = raw_disk->block_size;
 	}

+	/* If _disk_analyse_block_size has increased the sector_size,
+	   reallocate this buffer, so we can still read a sector into it.  */
+	if (ss0 < disk->dev->sector_size) {
+		free (buf);
+		buf = ped_malloc (disk->dev->sector_size);
+		if (buf == NULL)
+			goto error;
+	}
+
 	for (num=1; num==1 || num <= last_part_entry_num; num++) {
 		void *raw_part = buf;
 		if (!ped_device_read (disk->dev, raw_part,
--
1.7.12.289.g0ce9864