potential memory overrun in ata_id_to_hd_driveid() on big endian machines

potential memory overrun in ata_id_to_hd_driveid() on big endian machines

[Christian Engelmayer]

Please have a look at the usage of ata_id_to_hd_driveid() in ide-ioctls.c.

u16 array 'id' is allocated depending on the command, which might result in
142 byte. Indexing into the array at position 'ATA_ID_LBA_CAPACITY_2' in
ata_id_to_hd_driveid() would overrun the allocated memory in that case.

Regards,
 Christian

ide-ioctls.c:

	static int ide_get_identity_ioctl(ide_drive_t *drive, unsigned int cmd,
					  unsigned long arg)

	u16 *id = NULL;
	int size = (cmd == HDIO_GET_IDENTITY) ? (ATA_ID_WORDS * 2) : 142;

	..

	id = kmalloc(size, GFP_KERNEL);

	..

	ata_id_to_hd_driveid(id);

	..

ata.h:

	static inline void ata_id_to_hd_driveid(u16 *id)
	{
	#ifdef __BIG_ENDIAN

	..

	*(u64 *)&id[ATA_ID_LBA_CAPACITY_2] =
		ata_id_u64(id, ATA_ID_LBA_CAPACITY_2);

Re: potential memory overrun in ata_id_to_hd_driveid() on big endian machines

[Robert Hancock]

On 06/19/2009 12:41 AM, Christian Engelmayer wrote:
> Please have a look at the usage of ata_id_to_hd_driveid() in ide-ioctls.c.
>
> u16 array 'id' is allocated depending on the command, which might result in
> 142 byte. Indexing into the array at position 'ATA_ID_LBA_CAPACITY_2' in
> ata_id_to_hd_driveid() would overrun the allocated memory in that case.

Looks like ata_id_to_hd_driveid assumes the id memory is fully 
allocated, which seems a reasonable assumption. ide_get_identity_ioctl 
should likely allocate the full ATA_ID_WORDS * 2 unconditionally even if 
only part of it is used.

>
> Regards,
>   Christian
>
> ide-ioctls.c:
>
> 	static int ide_get_identity_ioctl(ide_drive_t *drive, unsigned int cmd,
> 					  unsigned long arg)
>
> 	u16 *id = NULL;
> 	int size = (cmd == HDIO_GET_IDENTITY) ? (ATA_ID_WORDS * 2) : 142;
>
> 	..
>
> 	id = kmalloc(size, GFP_KERNEL);
>
> 	..
>
> 	ata_id_to_hd_driveid(id);
>
> 	..
>
> ata.h:
>
> 	static inline void ata_id_to_hd_driveid(u16 *id)
> 	{
> 	#ifdef __BIG_ENDIAN
>
> 	..
>
> 	*(u64 *)&id[ATA_ID_LBA_CAPACITY_2] =
> 		ata_id_u64(id, ATA_ID_LBA_CAPACITY_2);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 1/1] ide: memory overrun in ide_get_identity_ioctl() on big endian machines using ioctl HDIO_OBSOLETE_IDENTITY

[Christian Engelmayer]

From: Christian Engelmayer <christian.engelmayer@frequentis.com>

This patch fixes a memory overrun in function ide_get_identity_ioctl() which
chooses the size of a memory buffer depending on the ioctl command that led
to the function call, however, passes that buffer to a function which needs the
buffer size to be always chosen unconditionally.

Due to conditional compilation the memory overrun can only happen on big endian
machines. The error can be triggered using ioctl HDIO_OBSOLETE_IDENTITY. Usage
of ioctl HDIO_GET_IDENTITY is safe.

Signed-off-by: Christian Engelmayer <christian.engelmayer@frequentis.com>
--
Proposed patch after comment by Robert Hancock who shares the view that buffer
'id' should be allocated unconditionally.

--- drivers/ide/ide-ioctls.c.orig	2009-06-20 23:22:45.000000000 +0200
+++ drivers/ide/ide-ioctls.c	2009-06-20 23:30:21.000000000 +0200
@@ -64,7 +64,8 @@ static int ide_get_identity_ioctl(ide_dr
 		goto out;
 	}
 
-	id = kmalloc(size, GFP_KERNEL);
+	/* ata_id_to_hd_driveid() relies on 'id' to be fully allocated. */
+	id = kmalloc(ATA_ID_WORDS * 2, GFP_KERNEL);
 	if (id == NULL) {
 		rc = -ENOMEM;
 		goto out;

Re: [PATCH 1/1] ide: memory overrun in ide_get_identity_ioctl() on big endian machines using ioctl HDIO_OBSOLETE_IDENTITY

[Bartlomiej Zolnierkiewicz]

On Sunday 21 June 2009 00:04:23 Christian Engelmayer wrote:
> From: Christian Engelmayer <christian.engelmayer@frequentis.com>
> 
> This patch fixes a memory overrun in function ide_get_identity_ioctl() which
> chooses the size of a memory buffer depending on the ioctl command that led
> to the function call, however, passes that buffer to a function which needs the
> buffer size to be always chosen unconditionally.
> 
> Due to conditional compilation the memory overrun can only happen on big endian
> machines. The error can be triggered using ioctl HDIO_OBSOLETE_IDENTITY. Usage
> of ioctl HDIO_GET_IDENTITY is safe.
> 
> Signed-off-by: Christian Engelmayer <christian.engelmayer@frequentis.com>

Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

> --
> Proposed patch after comment by Robert Hancock who shares the view that buffer
> 'id' should be allocated unconditionally.
> 
> --- drivers/ide/ide-ioctls.c.orig	2009-06-20 23:22:45.000000000 +0200
> +++ drivers/ide/ide-ioctls.c	2009-06-20 23:30:21.000000000 +0200
> @@ -64,7 +64,8 @@ static int ide_get_identity_ioctl(ide_dr
>  		goto out;
>  	}
>  
> -	id = kmalloc(size, GFP_KERNEL);
> +	/* ata_id_to_hd_driveid() relies on 'id' to be fully allocated. */
> +	id = kmalloc(ATA_ID_WORDS * 2, GFP_KERNEL);
>  	if (id == NULL) {
>  		rc = -ENOMEM;
>  		goto out;

Re: [PATCH 1/1] ide: memory overrun in ide_get_identity_ioctl() on big endian machines using ioctl HDIO_OBSOLETE_IDENTITY

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Mon, 22 Jun 2009 11:49:14 +0200

> On Sunday 21 June 2009 00:04:23 Christian Engelmayer wrote:
>> From: Christian Engelmayer <christian.engelmayer@frequentis.com>
>> 
>> This patch fixes a memory overrun in function ide_get_identity_ioctl() which
>> chooses the size of a memory buffer depending on the ioctl command that led
>> to the function call, however, passes that buffer to a function which needs the
>> buffer size to be always chosen unconditionally.
>> 
>> Due to conditional compilation the memory overrun can only happen on big endian
>> machines. The error can be triggered using ioctl HDIO_OBSOLETE_IDENTITY. Usage
>> of ioctl HDIO_GET_IDENTITY is safe.
>> 
>> Signed-off-by: Christian Engelmayer <christian.engelmayer@frequentis.com>
> 
> Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Applied, thanks everyone.