Date: Fri, 7 Jul 2017 17:28:34 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: prameela.j04cs@gmail.com
Cc: linux-wireless@vger.kernel.org,
        Amitkumar Karwar <amit.karwar@redpinesignals.com>
Subject: Re: [bug report] rsi: Add new host interface operations
Message-ID: <20170707142834.46l26uc2yrcxyidk@mwanda> (sfid-20170707_162852_330580_EA8318B7)
References: <20170612195908.GA2079@elgon.mountain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
In-Reply-To: <20170612195908.GA2079@elgon.mountain>
Sender: linux-wireless-owner@vger.kernel.org

None of these issues have been addressed.

regards,
dan carpenter

On Mon, Jun 12, 2017 at 10:59:08PM +0300, Dan Carpenter wrote:
> Hello Prameela Rani Garnepudi,
> 
> The patch b97e9b94ad75: "rsi: Add new host interface operations" from
> May 16, 2017, leads to the following static checker warning:
> 
> 	drivers/net/wireless/rsi/rsi_91x_usb.c:400 rsi_usb_master_reg_read()
> 	warn: passing casted pointer 'value' to 'rsi_usb_reg_read()' 32 vs 16.
> 
> drivers/net/wireless/rsi/rsi_91x_usb.c
>    156  static int rsi_usb_reg_read(struct usb_device *usbdev,
>    157                              u32 reg,
>    158                              u16 *value,
>    159                              u16 len)
>    160  {
>    161          u8 *buf;
>    162          int status = -ENOMEM;
>    163  
>    164          buf  = kmalloc(0x04, GFP_KERNEL);
> 
> Why are we allocating 0x4 bytes?
> 
>    165          if (!buf)
>    166                  return status;
>    167  
>    168          status = usb_control_msg(usbdev,
>    169                                   usb_rcvctrlpipe(usbdev, 0),
>    170                                   USB_VENDOR_REGISTER_READ,
>    171                                   RSI_USB_REQ_IN,
>    172                                   ((reg & 0xffff0000) >> 16), (reg & 0xffff),
>    173                                   (void *)buf,
>    174                                   len,
>                                          ^^^
> If len is more than 4 then we have memory corruption.
> 
>    175                                   USB_CTRL_GET_TIMEOUT);
>    176  
>    177          *value = (buf[0] | (buf[1] << 8));
>    178          if (status < 0) {
>    179                  rsi_dbg(ERR_ZONE,
>    180                          "%s: Reg read failed with error code :%d\n",
>    181                          __func__, status);
>    182          }
> 
> [ snip ]
> 
>    394  static int rsi_usb_master_reg_read(struct rsi_hw *adapter, u32 reg,
>    395                                     u32 *value, u16 len)
>    396  {
>    397          struct usb_device *usbdev =
>    398                  ((struct rsi_91x_usbdev *)adapter->rsi_dev)->usbdev;
>    399  
>    400          return rsi_usb_reg_read(usbdev, reg, (u16 *)value, len);
> 
> This is an endian bug because we're assuming the CPU is little endian.
> 
> 	u16 tmp;
> 	int ret;
> 
> 	if (len != sizeof(*value)) /* Or maybe sizeof(tmp), who knows? */
> 		return -EINVAL;
> 	ret = rsi_usb_reg_read(usbdev, reg, &tmp, len);
> 	if (ret)
> 		return ret;
> 
> 	*value = tmp;
> 	return 0;
> 
> 
>    401  }
> 
> regards,
> dan carpenter