Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        Pavel Skripkin <paskripkin@gmail.com>
Cc:     Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
        gregkh@linuxfoundation.org, straube.linux@gmail.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32
Date:   Tue, 24 Aug 2021 17:07:02 +0200
Message-ID: <1655559.bbUi69aIZD@localhost.localdomain>
In-Reply-To: <e50d266f-410c-34cb-e28f-f27dc429ef37@gmail.com>
References: <cover.1629642658.git.paskripkin@gmail.com> <20210824065825.GL1931@kadam> <e50d266f-410c-34cb-e28f-f27dc429ef37@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On Tuesday, August 24, 2021 9:01:23 AM CEST Pavel Skripkin wrote:
> On 8/24/21 9:58 AM, Dan Carpenter wrote:
> > On Sun, Aug 22, 2021 at 05:36:01PM +0300, Pavel Skripkin wrote:
> >> -static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
> >> +static int usb_read32(struct intf_hdl *pintfhdl, u32 addr, u32 *data)
> >>  {
> >>  	u8 requesttype;
> >>  	u16 wvalue;
> >>  	u16 len;
> >> -	__le32 data;
> >> +	int res;
> >> +	__le32 tmp;
> >> +
> >> +	if (WARN_ON(unlikely(!data)))
> >> +		return -EINVAL;
> >>  
> >>  	requesttype = 0x01;/* read_in */
> >>  
> >>  	wvalue = (u16)(addr & 0x0000ffff);
> >>  	len = 4;
> >>  
> >> -	usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> >> +	res = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> >> +	if (res < 0) {
> >> +		dev_err(dvobj_to_dev(pintfhdl->pintf_dev), "Failed to read 32 bytes: %d\n", res);
> > 
> > Add a return here.  Try to keep the success path and the failure path
> > as separate as possible.  Try to keep the success path indented at one
> > tab so the code looks like this:
> > 
> > 	success();
> > 	success();
> > 	if (fail)
> > 		handle_failure();
> > 	success();
> > 	success();
> > 
> > Try to deal with exceptions as quickly as possible so that the reader
> > has less to remember.
> > 
> >> +	} else {
> >> +		/* Noone cares about positive return value */
> > 
> > Ugh...  That's unfortunate.  We should actually care.  The
> > usbctrl_vendorreq() has an information leak where it copies len (4)
> > bytes of data even if usb_control_msg() is not able to read len bytes.
> > 
> > The best fix would be to remove the information leak and make
> > usbctrl_vendorreq() return zero on success.  In other words something
> > like:
> > 
> > 	status = usb_control_msg();
> > 	if (status < 0)
> > 		return status;
> > 	if (status != len)
> > 		return -EIO;
> > 	status = 0;
> > 
> 
> I see, thank you for reviewing, will fix in v3! I fully forgot, that 
> usb_control_msg() can receive only part of the message :)

With the use of the new API I think that you don't have anymore
partial messages. usb_control_msg() returns the number of bytes 
transferred, while usb_control_msg_recv/send return only 0 if successful
otherwise a negative error number.

Regards,

Fabio

> With regards,
> Pavel Skripkin
>