From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754294AbZIKJl3 (ORCPT ); Fri, 11 Sep 2009 05:41:29 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753049AbZIKJl2 (ORCPT ); Fri, 11 Sep 2009 05:41:28 -0400 Received: from mail-px0-f189.google.com ([209.85.216.189]:37171 "EHLO mail-px0-f189.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751103AbZIKJl2 convert rfc822-to-8bit (ORCPT ); Fri, 11 Sep 2009 05:41:28 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=A5fANbzku7dCANA9JVOSME8pe8pYroFEFvO79ttOjdtz78tgmD6moe9DXMWsJAkkzU +Ei+ZUaH4ZTL8LYiMmtVSbrXq0/jFPZMmTGj6xJ46hD+eKbt4Su0yqsWkkhp9uXhkrR1 NVL4g4/srY/+WDE90gm3gg7Ze18QL6ynbnwmE= MIME-Version: 1.0 In-Reply-To: References: Date: Fri, 11 Sep 2009 17:41:30 +0800 Message-ID: Subject: Re: [PATCH] remove variable length array tuner-xc2028 From: ye janboe To: Jesper Juhl Cc: dwmw2@infradead.org, linux-kernel Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org thank you for your comments, Jesper I will update the patch later after I fix the issue gmail wrap my patch:-( 2009/9/11 Jesper Juhl : > 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 > >