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

[Dan Carpenter <dan.carpenter@oracle.com>]

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