[Stgt-devel] [PATCH spc/mmc 4/4] RESEND modesense memory corruption bug

[FUJITA Tomonori]

On Tue, 6 May 2008 16:50:40 +1000
"ronnie sahlberg" <ronniesahlberg at gmail.com> wrote:

> >From 37275d5cfafc87a165824cbf9722ea742a770403 Mon Sep 17 00:00:00 2001
> From: Ronnie Sahlberg <ronniesahlberg at gmail.com>
> Date: Tue, 6 May 2008 16:37:07 +1000
> Subject: [PATCH] Fix a memory corruption bug in build_mode_page
> 
> If an application is providing a small allocation size when asking for a large
> mode page hte memcpy() in build_mode_page can cause memory to be overwritten
> and cause a crash.
> 
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg at gmail.com>
> ---
>  usr/spc.c |   63 ++++++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 36 insertions(+), 27 deletions(-)

Thanks a lot,


> diff --git a/usr/spc.c b/usr/spc.c
> index e3e4d98..35de2e1 100644
> --- a/usr/spc.c
> +++ b/usr/spc.c
> @@ -335,22 +335,28 @@ int spc_test_unit(int host_no, struct scsi_cmd *cmd)
>   *
>   * Returns number of bytes copied.
>   */
> -static int build_mode_page(uint8_t *data, struct mode_pg *pg, int update)
> +static uint8_t *build_mode_page(uint8_t *data, uint8_t *p, int alloc_len,
> +struct mode_pg *pg)
>  {
> -	uint8_t *p;
> -	int len;
> +	int i;
> +
> +	if (alloc_len > (p-data))
> +		*p = pg->pcode;
> +	p++;
> +
> +
> +	if (alloc_len > (p-data))
> +		*p = pg->pcode_size;
> +	p++;
> +
> 
> -	len = pg->pcode_size;
> -	if (update) {
> -		data[0] = pg->pcode;
> -		data[1] = len;
> +	for (i = 0; i < pg->pcode_size; i++) {
> +		if (alloc_len > (p-data))
> +			*p = pg->mode_data[i];
> +		p++;
>  	}
> -	p = &data[2];
> -	len += 2;
> -	if (update)
> -		memcpy(p, pg->mode_data, pg->pcode_size);
> 
> -	return len;
> +	return p;
>  }
> 
>  /**
> @@ -362,12 +368,13 @@ static int build_mode_page(uint8_t *data, struct
> mode_pg *pg, int update)
>   */
>  int spc_mode_sense(int host_no, struct scsi_cmd *cmd)
>  {
> -	int len = 0;
>  	uint8_t *data = NULL, *scb, mode6, dbd, pcode, subpcode;
> +	uint8_t *p;
>  	uint16_t alloc_len;
>  	unsigned char key = ILLEGAL_REQUEST;
>  	uint16_t asc = ASC_INVALID_FIELD_IN_CDB;
>  	struct mode_pg *pg;
> +	int i;
> 
>  	scb = cmd->scb;
>  	mode6 = (scb[0] == 0x1a);
> @@ -383,10 +390,10 @@ int spc_mode_sense(int host_no, struct scsi_cmd *cmd)
> 
>  	if (mode6) {
>  		alloc_len = scb[4];
> -		len = 4;
> +		p = &data[4];
>  	} else {
>  		alloc_len = (scb[7] << 8) + scb[8];
> -		len = 8;
> +		p = &data[8];
>  	}
>  	if (scsi_get_in_length(cmd) < alloc_len)

We still have the same problem here? If a buggy (or malicious)
initiator sends a bogus cdb, alloc_len can be larger than what we
actually allocated.

I'll fix this bug later.