linux-kernel - Re: [PATCH] rts5208: fix a missing check of the status of sd_init

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: <20190102102814.GF3781@kadam>
Date:   Wed, 2 Jan 2019 13:28:14 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Kangjie Lu <kjlu@....edu>
Cc:     devel@...verdev.osuosl.org, Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, pakki001@....edu,
        Colin Ian King <colin.king@...onical.com>,
        Aymen Qader <qader.aymen@...il.com>
Subject: Re: [PATCH] rts5208: fix a missing check of the status of
 sd_init_power

On Tue, Dec 25, 2018 at 02:33:32AM -0600, Kangjie Lu wrote:
> sd_init_power() could fail. The fix inserts a check of its status. If it
> fails, returns STATUS_FAIL.
> 
> Signed-off-by: Kangjie Lu <kjlu@....edu>
> ---
>  drivers/staging/rts5208/sd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
> index ff1a9aa152ce..8c3fd885a4f3 100644
> --- a/drivers/staging/rts5208/sd.c
> +++ b/drivers/staging/rts5208/sd.c
> @@ -2352,7 +2352,9 @@ static int reset_sd(struct rtsx_chip *chip)
>  				break;
>  			}
>  
> -			sd_init_power(chip);
> +			retval = sd_init_power(chip);
> +			if (retval != STATUS_SUCCESS)
> +				goto status_fail;

Are you able to test this?

I don't think you appreciate the risk of applying this patch.  We are
taking code which succeeds and making it fail.  That's a risky thing.
The benefit is just that it makes a static analysis tool happy?

It would be different if the benefit were that it improves security or
prevents a crash.  Or if you could test it.

This is the reset_sd() function.  It always feels a bit hacky to me to
even have a reset function.  It basically means you know you have messed
up but you don't know where or how so let's just try scrub everything
and go back to the start.  Maybe the driver is trying to work around a
firmware bug.  I often find bugs in reset functions which show they have
not been tested...  It's so tricky for me to do a cost benefit analysis
here, but I sort of think we should leave the code as-is.

It's frustrating because the static analysis tool is probably right, but
maybe the code only works because two bugs cancel each other out?  We
can't know without testing.

In a more ideal world we would do the static analysis on patches before
they are applied and refuse to apply them until the maintainer explains
why the function is not checked.

regards,
dan carpenter