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

[Jim Meyering]

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);
 }