[parted-devel] aix.c: Avoid memory overrun. Don't assume logical sector size <= 512B

[David Cantrell]

On Thu, 2007-03-08 at 22:26 +0100, Jim Meyering wrote:
> David Cantrell <dcantrell at redhat.com> wrote:
> > On Thu, 2007-03-08 at 15:43 +0100, Jim Meyering wrote:
> >> Here's a fix for the first memory overrun bug I found:
> >> 
> >> 	aix.c: Avoid memory overrun.  Don't assume logical sector size <= 512B
> >> 	* libparted/labels/aix.c (aix_probe): Return 0 if the
> >> 	sector size is larger than our AixLabel size.
> >> 	(aix_clobber): Rather than PED_ASSERT'ing that aix_probe returns 1,
> >> 	simply return 0 if aix_probe returns fails.
> ...
> > I like the approach of allocating the required buffer size instead of
> > using fixed-size buffers.
> >
> > Either solution is fine, but I would prefer we go with the ped_malloc()
> > approach that's used in amiga_probe so we are consistent throughout
> > libparted.
> 
> OK.  I've reworked it and tested the read-only parts.
> I have *not* tested the aix_clobber change: no disposable
> spindle handy right now.
> 
> 	aix.c: Avoid memory overrun.  Don't assume logical sector size <= 512B
> 	* libparted/labels/aix.c (struct AixLabel): Remove definition.
> 	(aix_label_magic_get, aix_label_magic_set): New functions.
> 	(read_sector): New function.
> 	(aix_probe): Rewrite not to use the above, and not a static buffer.
> 	(aix_clobber): Likewise.
> 	Also, rather than PED_ASSERT'ing that aix_probe returns 1,
> 	simply return 0 if aix_probe returns fails.
> 	(ped_disk_aix_init): Remove assertion, now that AixLabel is gone.
> 
> diff --git a/libparted/labels/aix.c b/libparted/labels/aix.c
> index a16ead4..e001617 100644
> --- a/libparted/labels/aix.c
> +++ b/libparted/labels/aix.c
> @@ -33,45 +33,68 @@
>  #  define _(String) (String)
>  #endif /* ENABLE_NLS */
> 
> -typedef struct {
> -	unsigned int   magic;        /* expect AIX_LABEL_MAGIC */
> -	unsigned int   fillbytes[127];
> -} AixLabel;
> -
>  #define	AIX_LABEL_MAGIC		0xc9c2d4c1
> 
>  static PedDiskType aix_disk_type;
> 
> -static int
> -aix_probe (const PedDevice *dev)
> +static inline int
> +aix_label_magic_get (const char *label)
>  {
> -	AixLabel	label;
> +	return *(int *)label;
> +}
> 
> -	PED_ASSERT (dev != NULL, return 0);
> +static inline void
> +aix_label_magic_set (char *label, int magic_val)
> +{
> +	*(int *)label = magic_val;
> +}
> 
> -	if (!ped_device_read (dev, &label, 0, 1))
> +/* Read a single sector, of length DEV->sector_size, into malloc'd storage.
> +   If the read fails, free the memory and return zero without modifying *BUF.
> +   Otherwise, set *BUF to the new buffer and return 1.  */
> +static int
> +read_sector (const PedDevice *dev, char **buf)
> +{
> +	char *b = ped_malloc (dev->sector_size);
> +	PED_ASSERT (b != NULL, return 0);
> +	if (!ped_device_read (dev, b, 0, 1)) {
> +		ped_free (b);
>  		return 0;
> +	}
> +	*buf = b;
> +	return 1;
> +}
> 
> -	if (PED_BE32_TO_CPU (label.magic) != AIX_LABEL_MAGIC)
> -		return 0;
> +static int
> +aix_probe (const PedDevice *dev)
> +{
> +	PED_ASSERT (dev != NULL, return 0);
> 
> -	return 1;
> +	char *label;
> +	if (!read_sector (dev, &label))
> +		return 0;
> +	int magic = aix_label_magic_get (label);
> +	ped_free (label);
> +	return magic == AIX_LABEL_MAGIC;
>  }
> 
>  #ifndef DISCOVER_ONLY
>  static int
>  aix_clobber (PedDevice* dev)
>  {
> -	AixLabel label;
> -
>  	PED_ASSERT (dev != NULL, return 0);
> -	PED_ASSERT (aix_probe (dev), return 0);
> 
> -	if (!ped_device_read (dev, &label, 0, 1))
> +	if (!aix_probe (dev))
>  		return 0;
> -	
> -	label.magic = 0;
> -	return ped_device_write (dev, &label, 0, 1);
> +
> +	char *label;
> +	if (!read_sector (dev, &label))
> +		return 0;
> +
> +	aix_label_magic_set (label, 0);
> +	int result = ped_device_write (dev, label, 0, 1);
> +	ped_free (label);
> +	return result;
>  }
>  #endif /* !DISCOVER_ONLY */
> 
> @@ -263,7 +286,6 @@ static PedDiskType aix_disk_type = {
>  void
>  ped_disk_aix_init ()
>  {
> -	PED_ASSERT (sizeof (AixLabel) == 512, return);
>  	ped_disk_type_register (&aix_disk_type);
>  }

Should we not be using unsigned int where we're using int?

-- 
David Cantrell <dcantrell at redhat.com>
Red Hat / Westford, MA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20070308/fe76ecfe/attachment.pgp