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

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

[Rand Deeb]

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 (m1 != 0)
+				return (clock / m1);
+			break;
 		case SSB_CHIPCO_CLK_MC_M1M2:
-			return (clock / (m1 * m2));
+			if ((m1 * m2) != 0)
+				return (clock / (m1 * m2));
+			break;
 		case SSB_CHIPCO_CLK_MC_M1M2M3:
-			return (clock / (m1 * m2 * m3));
+			if ((m1 * m2 * m3) != 0)
+				return (clock / (m1 * m2 * m3));
+			break;
 		case SSB_CHIPCO_CLK_MC_M1M3:
-			return (clock / (m1 * m3));
+			if ((m1 * m3) != 0)
+				return (clock / (m1 * m3));
+			break;
 		}
 		return 0;
 	case SSB_PLLTYPE_2:
-- 
2.34.1

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

[Larry Finger]

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

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

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

On Wed, 30 Aug 2023 14:50:42 -0500
Larry Finger <Larry.Finger@lwfinger.net> wrote:

> 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;
>   }

Yes, I agree that this is the much simpler and also more sensible
solution to this theoretical problem.

-- 
Michael Büsch
https://bues.ch/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ранд Дееб]

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

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

[Krzysztof Kozlowski]

On 31/08/2023 09:07, Ранд Дееб wrote:
> 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.

Don't top -pots.

> 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 

Really? No, it does not work perfectly. Look at your email - you send it
non-standard way.

> 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.

What encryption? You mean encoding? Encryption is something entirely
else and if you enable PGP or whatever, you must disable it. It's public
work.

Best regards,
Krzysztof

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

[Ранд Дееб]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

----- On Aug 31, 2023, at 4:46 PM, Krzysztof Kozlowski krzk@kernel.org wrote:

> On 31/08/2023 09:07, Ранд Дееб wrote:
>> 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.
> 
> Don't top -pots.
> 
>> 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
> 
> Really? No, it does not work perfectly. Look at your email - you send it
> non-standard way.
> 
>> 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.
> 
> What encryption? You mean encoding? Encryption is something entirely
> else and if you enable PGP or whatever, you must disable it. It's public
> work.
> 
> Best regards,
> Krzysztof


Dear Krzysztof,

Thank you again for your professionalism, cooperation and 
recommendations, we appreciate it. I understand what you're saying, after 
I looked at the information in the link:
https://lore.kernel.org/all/20230830082759.23336-1-deeb.rand@confident.ru/raw

But what I am saying is that I sent the patch myself (gmail) and there 
was no encoding ! screenshot in the attachments !!
Anyway we will try to fix everything next time.

Regarding the ecoding/ecryption, I apologize, of course I know the 
difference, but it was a translator mistake, knowing that 
I do not usually use it, but for certain reasons I used it 
this time, I will be more careful next time.

"I hope this message will not be top-post"

Best regards,
Rand

[-- Attachment #2: send-email screenshot.png --]
[-- Type: image/png, Size: 23333 bytes --]

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

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On Thu, 31 Aug 2023 10:07:33 +0300 (MSK)
Ранд Дееб <deeb.rand@confident.ru> wrote:

> 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.


Yes, I understand your reasoning and I had the same thought initially.
But I looked into the code and I am pretty sure that there is no reason
for the default case returning 0.
In fact, I think returning a 1 as default makes much more sense as the
default value for a factor.

Changing this from 0 to 1 will get my ack.

-- 
Michael Büsch
https://bues.ch/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Larry Finger]

On 8/31/23 11:05, Michael Büsch wrote:
> On Thu, 31 Aug 2023 10:07:33 +0300 (MSK)
> Ранд Дееб <deeb.rand@confident.ru> wrote:
> 
>> 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.
> 
> 
> Yes, I understand your reasoning and I had the same thought initially.
> But I looked into the code and I am pretty sure that there is no reason
> for the default case returning 0.
> In fact, I think returning a 1 as default makes much more sense as the
> default value for a factor.
> 
> Changing this from 0 to 1 will get my ack.

As far as I can tell, that return 0 is there only to silence the compiler as the 
switch statement has no default. As I said earlier, there have been no reports 
of divide by zero entries in this ancient driver, thus the value returned is not 
relevant. As a static tool sees the possibility of a returned zero leading to a 
potential divide by zero, making it a one it the only thing that makes sense.

I have no idea why this part of the patch has led to so much discussion!

Larry

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

[Krzysztof Kozlowski]

On 30/08/2023 21:50, Larry Finger wrote:
> 
>   /* 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.

It's sending everything as quoted-printable. Nothing improved here from
their previous version.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 30/08/2023 10: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.

Version your patches and provide changelog after --- .

> 
> 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 (m1 !=3D 0)

Nothing improved here.

Don't send patches as quoted-printable via some weird mailers.
Recommendation is to use standard tool - git send-email.

As you can easily see on the web - it is re-formatted for quoted-printable:

https://lore.kernel.org/all/20230830082759.23336-1-deeb.rand@confident.ru/raw

Best regards,
Krzysztof

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

[Ранд Дееб]

Dear Krzysztof,

Thank you for your response, now I see, but we already used the standard 
tool git send-email to send the patch. It seems we have to set the 
transfer-encoding manually (not default), we will fix it next time.
It's our first time trying to send a patch.

Best Regards,
Rand

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

On 30/08/2023 10: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.

Version your patches and provide changelog after --- .

> 
> 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 (m1 !=3D 0)

Nothing improved here.

Don't send patches as quoted-printable via some weird mailers.
Recommendation is to use standard tool - git send-email.

As you can easily see on the web - it is re-formatted for quoted-printable:

https://lore.kernel.org/all/20230830082759.23336-1-deeb.rand@confident.ru/raw

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 31/08/2023 16:25, Ранд Дееб wrote:
> Dear Krzysztof,
> 
> Thank you for your response, now I see, but we already used the standard 
> tool git send-email to send the patch. It seems we have to set the 
> transfer-encoding manually (not default), we will fix it next time.
> It's our first time trying to send a patch.
> 

Again, do not top post.

If you used git send-email, then where is X-Mailer header saying it was
git send-email? That's why I doubt you used it.

Best regards,
Krzysztof

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

[Rand Deeb]

In the line 910, the value of m1 may be zero, so there is a possibility
of dividing by zero, we fixed it by checking the values before dividing
(found with SVACE). In the same way, after checking and reading the
function, we found that lines 906, 908, 912 have the same situation, so
we fixed them as well.

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 (m1 != 0)
+				return (clock / m1);
+			break;
 		case SSB_CHIPCO_CLK_MC_M1M2:
-			return (clock / (m1 * m2));
+			if ((m1 * m2) != 0)
+				return (clock / (m1 * m2));
+			break;
 		case SSB_CHIPCO_CLK_MC_M1M2M3:
-			return (clock / (m1 * m2 * m3));
+			if ((m1 * m2 * m3) != 0)
+				return (clock / (m1 * m2 * m3));
+			break;
 		case SSB_CHIPCO_CLK_MC_M1M3:
-			return (clock / (m1 * m3));
+			if ((m1 * m3) != 0)
+				return (clock / (m1 * m3));
+			break;
 		}
 		return 0;
 	case SSB_PLLTYPE_2:
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 29/08/2023 13:12, Rand Deeb wrote:
> In the line 910, the value of m1 may be zero, so there is a possibility
> of dividing by zero, we fixed it by checking the values before dividing
> (found with SVACE). In the same way, after checking and reading the
> function, we found that lines 906, 908, 912 have the same situation, so
> we fixed them as well.
> 
> 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 (m1 !=3D 0)

Address list does not look like patch for mainline, but anyway: Your
patch is corrupted. Fix your email setup.

Best regards,
Krzysztof