linux-kernel - Re: [PATCH] remove variable length array tuner-xc2028

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.0909111128250.31018@swampdragon.chaosbits.net>
Date:	Fri, 11 Sep 2009 11:33:46 +0200 (CEST)
From:	Jesper Juhl <jj@...osbits.net>
To:	ye janboe <janboe.ye@...il.com>
cc:	dwmw2@...radead.org, linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] remove variable length array tuner-xc2028

On Fri, 11 Sep 2009, ye janboe wrote:

> hi
> 
> using variable length array on kernel stack is not safe, so this patch
> using kmalloc instead variable length array in tuner-xc2028
> 
> Signed-off-by: janboe <janboe.ye@...il.com>
> ---
>  drivers/media/common/tuners/tuner-xc2028.c |   26 ++++++++++++++++----------
>  1 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/common/tuners/tuner-xc2028.c
> b/drivers/media/common/tuners/tuner-xc2028.c
> index 1adce9f..96259c5 100644
> --- a/drivers/media/common/tuners/tuner-xc2028.c
> +++ b/drivers/media/common/tuners/tuner-xc2028.c
> @@ -516,8 +516,8 @@ static int load_firmware(struct dvb_frontend *fe,
> unsigned int type,
>  			 v4l2_std_id *id)
>  {
>  	struct xc2028_data *priv = fe->tuner_priv;
> -	int                pos, rc;
> -	unsigned char      *p, *endp, buf[priv->ctrl.max_len];
> +	int                pos, rc, ret = -EINVAL;
> +	unsigned char      *p, *endp, *buf;
> 
>  	tuner_dbg("%s called\n", __func__);
> 
> @@ -532,6 +532,7 @@ static int load_firmware(struct dvb_frontend *fe,
> unsigned int type,
> 
>  	p = priv->firm[pos].ptr;
>  	endp = p + priv->firm[pos].size;
> +	buf = kmalloc(priv->ctrl.max_len, GFP_KERNEL);
> 

The call to kmalloc() could fail, so shouldn't we make this

     buf = kmalloc(priv->ctrl.max_len, GFP_KERNEL);
     if (!buf)
       return -ENOMEM;

or?


>  	while (p < endp) {
>  		__u16 size;
> @@ -539,14 +540,16 @@ static int load_firmware(struct dvb_frontend
> *fe, unsigned int type,
>  		/* Checks if there's enough bytes to read */
>  		if (p + sizeof(size) > endp) {
>  			tuner_err("Firmware chunk size is wrong\n");
> -			return -EINVAL;
> +			goto fail;
>  		}
> 
>  		size = le16_to_cpu(*(__u16 *) p);
>  		p += sizeof(size);
> 
> -		if (size == 0xffff)
> -			return 0;
> +		if (size == 0xffff) {
> +			ret = 0;
> +			goto fail;
> +		}
> 
>  		if (!size) {
>  			/* Special callback command received */
> @@ -554,7 +557,7 @@ static int load_firmware(struct dvb_frontend *fe,
> unsigned int type,
>  			if (rc < 0) {
>  				tuner_err("Error at RESET code %d\n",
>  					   (*p) & 0x7f);
> -				return -EINVAL;
> +				goto fail;
>  			}
>  			continue;
>  		}
> @@ -565,13 +568,13 @@ static int load_firmware(struct dvb_frontend
> *fe, unsigned int type,
>  				if (rc < 0) {
>  					tuner_err("Error at RESET code %d\n",
>  						  (*p) & 0x7f);
> -					return -EINVAL;
> +					goto fail;
>  				}
>  				break;
>  			default:
>  				tuner_info("Invalid RESET code %d\n",
>  					   size & 0x7f);
> -				return -EINVAL;
> +				goto fail;
> 
>  			}
>  			continue;
> @@ -586,7 +589,7 @@ static int load_firmware(struct dvb_frontend *fe,
> unsigned int type,
>  		if ((size + p > endp)) {
>  			tuner_err("missing bytes: need %d, have %d\n",
>  				   size, (int)(endp - p));
> -			return -EINVAL;
> +			goto fail;
>  		}
> 
>  		buf[0] = *p;
> @@ -603,7 +606,7 @@ static int load_firmware(struct dvb_frontend *fe,
> unsigned int type,
>  			rc = i2c_send(priv, buf, len + 1);
>  			if (rc < 0) {
>  				tuner_err("%d returned from send\n", rc);
> -				return -EINVAL;
> +				goto fail;
>  			}
> 
>  			p += len;
> @@ -611,6 +614,9 @@ static int load_firmware(struct dvb_frontend *fe,
> unsigned int type,
>  		}
>  	}
>  	return 0;

Why are we not freeing 'buf' here?
Previously it was an array on the stack and would be gone now and I don't 
see what has changed so that it should live on after we return here.


> +fail:
> +	kfree(buf);
> +	return ret;
>  }
> 
>  static int load_scode(struct dvb_frontend *fe, unsigned int type,


-- 
Jesper Juhl <jj@...osbits.net>             http://www.chaosbits.net/
Plain text mails only, please      http://www.expita.com/nomime.html
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/