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

[ye janboe <janboe.ye@gmail.com>]

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 <jj@chaosbits.net>:
> 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@gmail.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@chaosbits.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
>
>