On Thu, Apr 14, 2022 at 06:42:15PM +0300, Dan Carpenter wrote:
> > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c
> > index f984a5ab2c6f..e321ca4453ca 100644
> > --- a/drivers/staging/rtl8712/usb_ops_linux.c
> > +++ b/drivers/staging/rtl8712/usb_ops_linux.c
> > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value,
> > }
> > status = usb_control_msg(udev, pipe, request, reqtype, value, index,
> > pIo_buf, len, 500);
> > - if (status > 0) { /* Success this control transfer. */
> > - if (requesttype == 0x01) {
> > - /* For Control read transfer, we have to copy the read
> > - * data from pIo_buf to pdata.
> > - */
> > - memcpy(pdata, pIo_buf, status);
> > + /* For Control read transfer, copy the read data from pIo_buf to pdata
> > + * when control transfer success; otherwise init *pdata with 0.
> > + */
> > + if (requesttype == 0x01) {
> > + if (status > 0)
> > + memcpy(pdata, pIo_buf, status);
> > + else
> > + *(u32 *)pdata = 0;
> > }
>
> This isn't really correct. In many cases status is "len" is less than 4.
> I'm slightly surprised that nothing complains about that as an
> uninitialized access. But then another problem is that "status" can be
> less than "len".
>
> A better fix instead of setting pdata to zero would be to add error
> checking in the callers and then change this code to use
> usb_control_msg_send/recv(). Probably just initialize "data" in the
> callers as well.
A different, less good option would be to, still add error handling in
the callers, but do:
status = usb_control_msg();
if (status < 0)
goto free;
if (status != len) {
status = -EREMOTEIO;
goto free;
}
if (requesttype == 0x01)
memcpy(pdata, pIo_buf, status);
status = 0;
free:
kfree(palloc_buf);
return status;
regards,
dan carpenter
