[parted-devel] Patch: Fix gpt_read to read all of the partition entries correctly

[Jim Meyering]

"Matthew S. Harris" <mharris312 at gmail.com> wrote:
> Here's my one remaining fix.  Yes, I've been sitting on this one since
> April 2007 too.  Hopefully it's clear why this one is important.

Thanks!
I don't want to block on test cases,
but it'd sure make it easier to take a patch that
comes with a small test case to exercise/demonstrate the fix.

For example, if you can construct a tiny image,
describe how to perturb it, and give a parted
command that misbehaves as a result, yet that works
with your patch, that'd be great.

> From 282be216499436242cdb3083c0e5d9b672db8682 Mon Sep 17 00:00:00 2001
> From: Matthew S. Harris <mharris312 at gmail.com>
> Date: Tue, 2 Dec 2008 19:53:08 -0800
> Subject: [PATCH] Fix gpt_read to read all of the partition entries correctly.
>
> * libparted/labels/gpt.c (gpt_read): Use the SizeOfPartitionEntry
> field value when reading the partition entries rather than assuming
> that the entries are the same size as our struct.
>
> * libparted/labels/gpt.c (gpt_read): When reading the partition
> entries, round up, not down, the number of sectors to read.
> ---
>  libparted/labels/gpt.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
> index 13d2e88..892e921 100644
> --- a/libparted/labels/gpt.c
> +++ b/libparted/labels/gpt.c
> @@ -807,8 +807,8 @@ gpt_read (PedDisk * disk)
>  {
>  	GPTDiskData *gpt_disk_data = disk->disk_specific;
>  	GuidPartitionTableHeader_t* gpt;
> -	GuidPartitionEntry_t* ptes;
> -	int ptes_size;
> +	void* ptes;
> +	int ptes_sectors;
>  	int i;
>  #ifndef DISCOVER_ONLY
>  	int write_back = 0;
> @@ -889,21 +889,24 @@ gpt_read (PedDisk * disk)
>  		goto error_free_gpt;
>
>
> -	ptes_size = sizeof (GuidPartitionEntry_t) * gpt_disk_data->entry_count;
> -	ptes = (GuidPartitionEntry_t*) ped_malloc (ptes_size);
> +	ptes_sectors = ped_div_round_up(gpt->SizeOfPartitionEntry *
> +		gpt_disk_data->entry_count, disk->dev->sector_size);

Does something already ensure that SizeOfPartitionEntry is sane?
i.e., what if the on-disk value is invalid?
Maybe add a sanity check, if there isn't one already.

> +	ptes = ped_malloc (ptes_sectors * disk->dev->sector_size);

Before doing the malloc, ensure that the product does not overflow,
e.g.,

  if (xalloc_oversized (ptes_sectors, disk->dev->sector_size)
      goto ...;

>  	if (!ped_device_read (disk->dev, ptes,
>  			      PED_LE64_TO_CPU(gpt->PartitionEntryLBA),
> -			      ptes_size / disk->dev->sector_size))
> +			      ptes_sectors))
>  		goto error_free_ptes;
>
>  	for (i = 0; i < gpt_disk_data->entry_count; i++) {
> +		GuidPartitionEntry_t* pte = (GuidPartitionEntry_t*) (ptes +
> +			i * gpt->SizeOfPartitionEntry);
>  		PedPartition* part;
>  		PedConstraint* constraint_exact;
>
> -		if (!guid_cmp (ptes[i].PartitionTypeGuid, UNUSED_ENTRY_GUID))
> +		if (!guid_cmp (pte->PartitionTypeGuid, UNUSED_ENTRY_GUID))
>  			continue;
>
> -		part = _parse_part_entry (disk, &ptes[i]);
> +		part = _parse_part_entry (disk, pte);
>  		if (!part)
>  			goto error_delete_all;