The sdio_read32() calls sd_read(), but does not handle the error if
sd_read() fails. This could lead to subsequent operations processing
invalid data. A proper implementation can be found in sdio_readN(),
which has an error handling for the sd_read().

Add error handling for the sd_read() to free tmpbuf and return error
code if sd_read() fails. This ensure that the memcpy() is only performed
when the read operation is successful.

Since none of the callers check for the errors, there is no need to
return the error code propagated from sd_read(). Returning SDIO_ERR_VAL32
might be a better choice, which is a specialized error code for SDIO.

Another problem of returning propagated error code is that the error
code is a s32 type value, which is not fit with the u32 type return value
of the sdio_read32().

An practical option would be to go through all the callers and add error
handling, which need to pass a pointer to u32 *val and return zero on
success or negative on failure. It is not a better choice since will cost
unnecessary effort on the error code.

The other opion is to replace sd_read() by sd_read32(), which return an
u32 type error code that can be directly used as the return value of
sdio_read32(). But, it is also a bad choice to use sd_read32() in a
alignment failed branch.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org # v4.12+
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
v7: Fix error code and add patch explanation
v6: Fix improper code to propagate error code
v5: Fix error code
v4: Add change log and fix error code
v3: Add Cc flag
v2: Change code to initialize val

 drivers/staging/rtl8723bs/hal/sdio_ops.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c
index 21e9f1858745..d79d41727042 100644
--- a/drivers/staging/rtl8723bs/hal/sdio_ops.c
+++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c
@@ -185,7 +185,12 @@ static u32 sdio_read32(struct intf_hdl *intfhdl, u32 addr)
 			return SDIO_ERR_VAL32;
 
 		ftaddr &= ~(u16)0x3;
-		sd_read(intfhdl, ftaddr, 8, tmpbuf);
+		err = sd_read(intfhdl, ftaddr, 8, tmpbuf);
+		if (err) {
+			kfree(tmpbuf);
+			return SDIO_ERR_VAL32;
+		}
+
 		memcpy(&le_tmp, tmpbuf + shift, 4);
 		val = le32_to_cpu(le_tmp);
 
-- 
2.42.0.windows.2

On Tue, Apr 08, 2025 at 12:41:52PM +0800, Wentao Liang wrote:
> The sdio_read32() calls sd_read(), but does not handle the error if
> sd_read() fails. This could lead to subsequent operations processing
> invalid data. A proper implementation can be found in sdio_readN(),
> which has an error handling for the sd_read().

Great, why not move to that instead?

> Add error handling for the sd_read() to free tmpbuf and return error
> code if sd_read() fails. This ensure that the memcpy() is only performed
> when the read operation is successful.
> 
> Since none of the callers check for the errors, there is no need to
> return the error code propagated from sd_read(). Returning SDIO_ERR_VAL32
> might be a better choice, which is a specialized error code for SDIO.

Again, fixing the callers would be best.

> Another problem of returning propagated error code is that the error
> code is a s32 type value, which is not fit with the u32 type return value
> of the sdio_read32().

Then that too should be fixed :)

> An practical option would be to go through all the callers and add error
> handling, which need to pass a pointer to u32 *val and return zero on
> success or negative on failure. It is not a better choice since will cost
> unnecessary effort on the error code.

I don't understand why this would be unnecessary effort, it would do the
right thing, correct?

> The other opion is to replace sd_read() by sd_read32(), which return an
> u32 type error code that can be directly used as the return value of
> sdio_read32(). But, it is also a bad choice to use sd_read32() in a
> alignment failed branch.

What do you mean by "alignment failed branch"?

> Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
> Cc: stable@vger.kernel.org # v4.12+

Why is this cc: stable?  Can you duplicate this problem on your system?
Have you tested this change?

> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
> v7: Fix error code and add patch explanation
> v6: Fix improper code to propagate error code
> v5: Fix error code
> v4: Add change log and fix error code
> v3: Add Cc flag
> v2: Change code to initialize val
> 
>  drivers/staging/rtl8723bs/hal/sdio_ops.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c
> index 21e9f1858745..d79d41727042 100644
> --- a/drivers/staging/rtl8723bs/hal/sdio_ops.c
> +++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c
> @@ -185,7 +185,12 @@ static u32 sdio_read32(struct intf_hdl *intfhdl, u32 addr)
>  			return SDIO_ERR_VAL32;
>  
>  		ftaddr &= ~(u16)0x3;
> -		sd_read(intfhdl, ftaddr, 8, tmpbuf);
> +		err = sd_read(intfhdl, ftaddr, 8, tmpbuf);
> +		if (err) {
> +			kfree(tmpbuf);
> +			return SDIO_ERR_VAL32;
> +		}

Again, I think this whole "hal wrapper" should be removed instead, and
not papered over like this.  If you dig deep enough, it all boils down
to a call to sdio_readb(), which is an 8 bit read, so the alignment
issues are not a problem, and if an error happens the proper error value
is returned from that saying what happened.  Why not work on that like I
recommended?  That would allow for at least 3, if not more, layers of
indirection to be removed from this driver, making it more easy to
understand and maintain over time.

thanks,

greg k-h