Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754327AbZIKJdp (ORCPT ); Fri, 11 Sep 2009 05:33:45 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754248AbZIKJdo (ORCPT ); Fri, 11 Sep 2009 05:33:44 -0400 Received: from swampdragon.chaosbits.net ([90.184.90.115]:25275 "EHLO swampdragon.chaosbits.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753677AbZIKJdo (ORCPT ); Fri, 11 Sep 2009 05:33:44 -0400 Date: Fri, 11 Sep 2009 11:33:46 +0200 (CEST) From: Jesper Juhl To: ye janboe cc: dwmw2@infradead.org, linux-kernel Subject: Re: [PATCH] remove variable length array tuner-xc2028 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4030 Lines: 145 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 > --- > 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 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/