Re: [PATCH] ssb-main: Fix division by zero in ssb_calc_clock_rate()

["Ранд Дееб" <deeb.rand@confident.ru>]

Dear Larry, 

1- Yes, I agree that your solution is simpler, and I thought about it, but 
I thought that the one who set the value zero put it for some reason, so I 
did not want to change it.
2- It's first time to send using this mailer, but we tested it by sending 
the patch to our personal emails (gmail, etc..) and it works perfectly! I 
don't know why this encryption appears only when we get a response from 
you, but we're still investigating. We will fix it, and follow all the 
recommendations in the future.

Thanks for your cooperation.

Best regards,
Rand

----- Original Message -----
From: "Larry Finger" <Larry.Finger@lwfinger.net>
To: "Rand Deeb" <deeb.rand@confident.ru>, "Michael Buesch" <m@bues.ch>, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: lvc-project@linuxtesting.org, "Воскресенский Станислав Игоревич" <voskresenski.stanislav@confident.ru>
Sent: Wednesday, August 30, 2023 10:50:42 PM
Subject: Re: [PATCH] ssb-main: Fix division by zero in ssb_calc_clock_rate()

On 8/30/23 03:27, Rand Deeb wrote:
> In ssb_calc_clock_rate(), the value of m1 may be zero because it is
> initialized using clkfactor_f6_resolv(). This function could return
> zero, so there is a possibility of dividing by zero, we fixed it by
> checking the values before dividing.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Rand Deeb <deeb.rand@confident.ru>
> ---
>   drivers/ssb/main.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index 0a26984acb2c..e0776a16d04d 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -903,13 +903,21 @@ u32 ssb_calc_clock_rate(u32 plltype, u32 n, u32 m)
>   		case SSB_CHIPCO_CLK_MC_BYPASS:
>   			return clock;
>   		case SSB_CHIPCO_CLK_MC_M1:
> -			return (clock / m1);
> +			if 
> +				return (clock / m1);
> +			break;
>   		case SSB_CHIPCO_CLK_MC_M1M2:
> -			return (clock / (m1 * m2));
> +			if ((m1 * m2) !=3D 0)
> +				return (clock / (m1 * m2));
> +			break;
>   		case SSB_CHIPCO_CLK_MC_M1M2M3:
> -			return (clock / (m1 * m2 * m3));
> +			if ((m1 * m2 * m3) !=3D 0)
> +				return (clock / (m1 * m2 * m3));
> +			break;
>   		case SSB_CHIPCO_CLK_MC_M1M3:
> -			return (clock / (m1 * m3));
> +			if ((m1 * m3) !=3D 0)
> +				return (clock / (m1 * m3));
> +			break;
>   		}
>   		return 0;
>   	case SSB_PLLTYPE_2:
> --=20
> 2.34.1

Rand,

I agree that clkfactor_f6_resolv() could return 0, but we have not been overrun 
with reports of divide by zero errors, which suggests that the branch is never 
taken. This patch will make your tool happy and is much simpler:

diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
index ab080cf26c9f..b9934b9c2d70 100644
--- a/drivers/ssb/main.c
+++ b/drivers/ssb/main.c
@@ -837,7 +837,7 @@ static u32 clkfactor_f6_resolve(u32 v)
         case SSB_CHIPCO_CLK_F6_7:
                 return 7;
         }
-       return 0;
+       return 1;
  }

  /* Calculate the speed the backplane would run at a given set of clockcontrol 
values */

Your patch has some technical problems as well. The subject should be "ssb: Fix 
division ..." In addition, note that all your if statements have an extraneous 
"3D" as in "(m1 !=3D 0)". To me, that indicates that your mailer is not sending 
plain text.

Larry