| Message ID | 20230830082759.23336-1-deeb.rand@confident.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ssb-main: Fix division by zero in ssb_calc_clock_rate() | expand |
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
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.
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
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
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
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
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
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
----- 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
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.
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
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:
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(-)