linux-kernel - Re: [PATCH] Staging: xgifb: XGI_main

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 20 Mar 2018 15:03:44 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Pratik Jain <pratik.jain0509@...il.com>
Cc:     arnaud.patard@...-net.org, devel@...verdev.osuosl.org,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

On Tue, Mar 20, 2018 at 02:05:49PM +0530, Pratik Jain wrote:
> Refactored the function `XGIfb_search_refresh_rate` by removing a level
> of `if...else` block nesting. Removed unnecessary parantheses.
> 
> Signed-off-by: Pratik Jain <pratik.jain0509@...il.com>
> ---
>  drivers/staging/xgifb/XGI_main_26.c | 63 +++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
> index 10107de0119a..ef9a726cd35d 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -544,41 +544,44 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
>  	yres = XGIbios_mode[xgifb_info->mode_idx].yres;
>  
>  	xgifb_info->rate_idx = 0;
> -	while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
> -		if ((XGIfb_vrate[i].xres == xres) &&
> -		    (XGIfb_vrate[i].yres == yres)) {
> -			if (XGIfb_vrate[i].refresh == rate) {
> +	

There is a stray tab here.  You didn't run checkpatch.pl.

> +	// Skip values with less xres

Linus likes this comment style, but I would prefer normal comments,
please.

> +	while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres < xres)
> +		++i;
> +

I have reviewed the code, and I still find the single loop more
readable.

> +	while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
> +		if (XGIfb_vrate[i].yres != yres) {
> +			++i;
> +			continue;
> +		}

I would like a change that did:

		if ((XGIfb_vrate[i].xres != xres) ||
		    (XGIfb_vrate[i].yres != yres)) {
			i++;
			continue;
		}

so we could pull everything in one tab.

> +		if (XGIfb_vrate[i].refresh == rate) {
> +			xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> +			break;
> +		} else if (XGIfb_vrate[i].refresh > rate) {
> +			if (XGIfb_vrate[i].refresh - rate <= 3) {
> +				pr_debug("Adjusting rate from %d up to %d\n",
> +					rate, XGIfb_vrate[i].refresh);
>  				xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> -				break;
> -			} else if (XGIfb_vrate[i].refresh > rate) {
> -				if ((XGIfb_vrate[i].refresh - rate) <= 3) {
> -					pr_debug("Adjusting rate from %d up to %d\n",
> -						 rate, XGIfb_vrate[i].refresh);
> -					xgifb_info->rate_idx =
> -						XGIfb_vrate[i].idx;
> -					xgifb_info->refresh_rate =
> -						XGIfb_vrate[i].refresh;
> -				} else if (((rate - XGIfb_vrate[i - 1].refresh)
> -						<= 2) && (XGIfb_vrate[i].idx
> -						!= 1)) {
> -					pr_debug("Adjusting rate from %d down to %d\n",
> -						 rate,
> -						 XGIfb_vrate[i - 1].refresh);
> -					xgifb_info->rate_idx =
> -						XGIfb_vrate[i - 1].idx;
> -					xgifb_info->refresh_rate =
> -						XGIfb_vrate[i - 1].refresh;
> -				}
> -				break;
> -			} else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
> +				xgifb_info->refresh_rate =
> +					XGIfb_vrate[i].refresh;
> +			} else if ((rate - XGIfb_vrate[i - 1].refresh <= 2)
> +					&& (XGIfb_vrate[i].idx != 1)) {
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This bug is there in the original code, and not something that you
introduced but the second part of the if condition is to ensure that
we didn't do an array underflow in the first part of the if statement.

These days that can trigger a kasan warning, I believe.

The conditions should be swapped around to avoid the read before the
start of the array altogether.  And, in fact, it should be written like
this to make it easier for static analysis tools:

			} else if (i != 0 &&
				   rate - XGIfb_vrate[i - 1].refresh <= 2) {

regards,
dan carpenter