linux-kernel - Re: [PATCH v7] staging: rtl8723bs: Add error handling for sd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2025040814-curtsy-overrule-1caf@gregkh>
Date: Tue, 8 Apr 2025 07:18:17 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Wentao Liang <vulab@...as.ac.cn>
Cc: philipp.g.hortmann@...il.com, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v7] staging: rtl8723bs: Add error handling for sd_read()

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@...r.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@...as.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