re: [media] drivers:media:radio: wl128x: FM Driver Common sources

re: [media] drivers:media:radio: wl128x: FM Driver Common sources

[Dan Carpenter]

Hello Manjunatha Halli,

The patch e8454ff7b9a4: "[media] drivers:media:radio: wl128x: FM
Driver Common sources" from Jan 11, 2011, leads to the following
warning:
drivers/media/radio/wl128x/fmdrv_common.c:596 fm_irq_handle_flag_getcmd_resp()
	 error: untrusted 'fm_evt_hdr->dlen' is not capped properly

[ this is on my private Smatch stuff with too many false positives for
  general release ].

   584  static void fm_irq_handle_flag_getcmd_resp(struct fmdev *fmdev)
   585  {
   586          struct sk_buff *skb;
   587          struct fm_event_msg_hdr *fm_evt_hdr;
   588  
   589          if (check_cmdresp_status(fmdev, &skb))
   590                  return;
   591  
   592          fm_evt_hdr = (void *)skb->data;
   593  
   594          /* Skip header info and copy only response data */
   595          skb_pull(skb, sizeof(struct fm_event_msg_hdr));
   596          memcpy(&fmdev->irq_info.flag, skb->data, fm_evt_hdr->dlen);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

   597  
   598          fmdev->irq_info.flag = be16_to_cpu(fmdev->irq_info.flag);
   599          fmdbg("irq: flag register(0x%x)\n", fmdev->irq_info.flag);
   600  
   601          /* Continue next function in interrupt handler table */
   602          fm_irq_call_stage(fmdev, FM_HW_MAL_FUNC_IDX);
   603  }

What are we copying here?  How do we know that ->dlen doesn't overflow
the buffer?  Why do we memcpy() and the overwrite part of the data on
the next line?

regards,
dan carpenter

Re: [media] drivers:media:radio: wl128x: FM Driver Common sources

[halli manjunatha]

On Fri, Jul 13, 2012 at 6:51 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Manjunatha Halli,
>
> The patch e8454ff7b9a4: "[media] drivers:media:radio: wl128x: FM
> Driver Common sources" from Jan 11, 2011, leads to the following
> warning:
> drivers/media/radio/wl128x/fmdrv_common.c:596
> fm_irq_handle_flag_getcmd_resp()
>          error: untrusted 'fm_evt_hdr->dlen' is not capped properly
>
> [ this is on my private Smatch stuff with too many false positives for
>   general release ].
>
>    584  static void fm_irq_handle_flag_getcmd_resp(struct fmdev *fmdev)
>    585  {
>    586          struct sk_buff *skb;
>    587          struct fm_event_msg_hdr *fm_evt_hdr;
>    588
>    589          if (check_cmdresp_status(fmdev, &skb))
>    590                  return;
>    591
>    592          fm_evt_hdr = (void *)skb->data;
>    593
>    594          /* Skip header info and copy only response data */
>    595          skb_pull(skb, sizeof(struct fm_event_msg_hdr));
>    596          memcpy(&fmdev->irq_info.flag, skb->data,
> fm_evt_hdr->dlen);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>    597
>    598          fmdev->irq_info.flag = be16_to_cpu(fmdev->irq_info.flag);
>    599          fmdbg("irq: flag register(0x%x)\n", fmdev->irq_info.flag);
>    600
>    601          /* Continue next function in interrupt handler table */
>    602          fm_irq_call_stage(fmdev, FM_HW_MAL_FUNC_IDX);
>    603  }
>
> What are we copying here?  How do we know that ->dlen doesn't overflow
> the buffer?  Why do we memcpy() and the overwrite part of the data on
> the next line?

Here I am trying to get the current value of the flag register which
is of maximum 16bit wide.

So ->dlen value never overflow the buffer.

In memcopy() case I am just trying to avoid 1 more variable so first I
memcopy the skb->data to  ->irq_info.flag then I will correct the
endianness of

->irq_info.flag in next line.

Regards
manju

>
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




--
Regards
Halli

Re: [media] drivers:media:radio: wl128x: FM Driver Common sources

[Dan Carpenter]

On Fri, Jul 13, 2012 at 01:17:11PM -0500, halli manjunatha wrote:
> On Fri, Jul 13, 2012 at 6:51 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Hello Manjunatha Halli,
> >
> > The patch e8454ff7b9a4: "[media] drivers:media:radio: wl128x: FM
> > Driver Common sources" from Jan 11, 2011, leads to the following
> > warning:
> > drivers/media/radio/wl128x/fmdrv_common.c:596
> > fm_irq_handle_flag_getcmd_resp()
> >          error: untrusted 'fm_evt_hdr->dlen' is not capped properly
> >
> > [ this is on my private Smatch stuff with too many false positives for
> >   general release ].
> >
> >    584  static void fm_irq_handle_flag_getcmd_resp(struct fmdev *fmdev)
> >    585  {
> >    586          struct sk_buff *skb;
> >    587          struct fm_event_msg_hdr *fm_evt_hdr;
> >    588
> >    589          if (check_cmdresp_status(fmdev, &skb))
> >    590                  return;
> >    591
> >    592          fm_evt_hdr = (void *)skb->data;
> >    593
> >    594          /* Skip header info and copy only response data */
> >    595          skb_pull(skb, sizeof(struct fm_event_msg_hdr));
> >    596          memcpy(&fmdev->irq_info.flag, skb->data,
> > fm_evt_hdr->dlen);
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> >    597
> >    598          fmdev->irq_info.flag = be16_to_cpu(fmdev->irq_info.flag);
> >    599          fmdbg("irq: flag register(0x%x)\n", fmdev->irq_info.flag);
> >    600
> >    601          /* Continue next function in interrupt handler table */
> >    602          fm_irq_call_stage(fmdev, FM_HW_MAL_FUNC_IDX);
> >    603  }
> >
> > What are we copying here?  How do we know that ->dlen doesn't overflow
> > the buffer?  Why do we memcpy() and the overwrite part of the data on
> > the next line?
> 
> Here I am trying to get the current value of the flag register which
> is of maximum 16bit wide.
> 
> So ->dlen value never overflow the buffer.
> 
> In memcopy() case I am just trying to avoid 1 more variable so first I
> memcopy the skb->data to  ->irq_info.flag then I will correct the
> endianness of
> 
> ->irq_info.flag in next line.
> 

I am sorry but your email makes no sense at all.  This is a remote
security hole because ->dlen is a u8 that is chosen from over the
network.  The memcpy would let us copy over some function pointers
in fmdev->irq_info->timer and use that to become root.

->dlen is not checked anywhere.

You say it copies 16 bits of data (in other words ->dlen is a
maximum of 2 (2 bytes).  fmdev->irq_info.flag is 16 bits.  In other
words the memcpy() can be removed.

Then you contradict yourself and say it copies other unspecified
data as well.

Can someone else take a look?

regards,
dan carpenter

Re: [media] drivers:media:radio: wl128x: FM Driver Common sources

[halli manjunatha]

On Fri, Jul 13, 2012 at 3:36 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Jul 13, 2012 at 01:17:11PM -0500, halli manjunatha wrote:
>> On Fri, Jul 13, 2012 at 6:51 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> >
>> > Hello Manjunatha Halli,
>> >
>> > The patch e8454ff7b9a4: "[media] drivers:media:radio: wl128x: FM
>> > Driver Common sources" from Jan 11, 2011, leads to the following
>> > warning:
>> > drivers/media/radio/wl128x/fmdrv_common.c:596
>> > fm_irq_handle_flag_getcmd_resp()
>> >          error: untrusted 'fm_evt_hdr->dlen' is not capped properly
>> >
>> > [ this is on my private Smatch stuff with too many false positives for
>> >   general release ].
>> >
>> >    584  static void fm_irq_handle_flag_getcmd_resp(struct fmdev *fmdev)
>> >    585  {
>> >    586          struct sk_buff *skb;
>> >    587          struct fm_event_msg_hdr *fm_evt_hdr;
>> >    588
>> >    589          if (check_cmdresp_status(fmdev, &skb))
>> >    590                  return;
>> >    591
>> >    592          fm_evt_hdr = (void *)skb->data;
>> >    593
>> >    594          /* Skip header info and copy only response data */
>> >    595          skb_pull(skb, sizeof(struct fm_event_msg_hdr));
>> >    596          memcpy(&fmdev->irq_info.flag, skb->data,
>> > fm_evt_hdr->dlen);
>> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >
>> >    597
>> >    598          fmdev->irq_info.flag = be16_to_cpu(fmdev->irq_info.flag);
>> >    599          fmdbg("irq: flag register(0x%x)\n", fmdev->irq_info.flag);
>> >    600
>> >    601          /* Continue next function in interrupt handler table */
>> >    602          fm_irq_call_stage(fmdev, FM_HW_MAL_FUNC_IDX);
>> >    603  }
>> >
>> > What are we copying here?  How do we know that ->dlen doesn't overflow
>> > the buffer?  Why do we memcpy() and the overwrite part of the data on
>> > the next line?
>>
>> Here I am trying to get the current value of the flag register which
>> is of maximum 16bit wide.
>>
>> So ->dlen value never overflow the buffer.
>>
>> In memcopy() case I am just trying to avoid 1 more variable so first I
>> memcopy the skb->data to  ->irq_info.flag then I will correct the
>> endianness of
>>
>> ->irq_info.flag in next line.
>>
>
> I am sorry but your email makes no sense at all.  This is a remote
> security hole because ->dlen is a u8 that is chosen from over the
> network.  The memcpy would let us copy over some function pointers
> in fmdev->irq_info->timer and use that to become root.
>
> ->dlen is not checked anywhere.
>
> You say it copies 16 bits of data (in other words ->dlen is a
> maximum of 2 (2 bytes).  fmdev->irq_info.flag is 16 bits.  In other
> words the memcpy() can be removed.
>
> Then you contradict yourself and say it copies other unspecified
> data as well.

Pardon my previous answer I didn’t checked the ->dlen.

Yes you are right ->dlen is just a u8 and flag is of 16 bits and I am
not checking the validity of ->dlen.

I will remove the memcpy() and submit the patch

Regards
manju
>
> Can someone else take a look?
>
> regards,
> dan carpenter



-- 
Regards
Halli