diff mbox series

[next] net: dsa: fix unintended sign extension on a u16 left shift

Message ID 20201109124008.2079873-1-colin.king@canonical.com
State Accepted
Delegated to: David Miller
Headers show
Series [next] net: dsa: fix unintended sign extension on a u16 left shift | expand

Checks

Context Check Description
jkicinski/verify_fixes success Link
jkicinski/cover_letter success Link
jkicinski/fixes_present success Link
jkicinski/patch_count success Link
jkicinski/tree_selection success Guessed tree name to be net-next
jkicinski/subject_prefix warning Target tree name not specified in the subject
jkicinski/source_inline success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/module_param success Was 0 now: 0
jkicinski/build_32bit success Errors and warnings before: 0 this patch: 0
jkicinski/kdoc success Errors and warnings before: 0 this patch: 0
jkicinski/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
jkicinski/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
jkicinski/header_inline success Link
jkicinski/stable success Stable not CCed

Commit Message

Colin King Nov. 9, 2020, 12:40 p.m. UTC
From: Colin Ian King <colin.king@canonical.com>

The left shift of u16 variable high is promoted to the type int and
then sign extended to a 64 bit u64 value.  If the top bit of high is
set then the upper 32 bits of the result end up being set by the
sign extension. Fix this by explicitly casting the value in high to
a u64 before left shifting by 16 places.

Also, remove the initialisation of variable value to 0 at the start
of each loop iteration as the value is never read and hence the
assignment it is redundant.

Addresses-Coverity: ("Unintended sign extension")
Fixes: e4b27ebc780f ("net: dsa: Add DSA driver for Hirschmann Hellcreek switches")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/net/dsa/hirschmann/hellcreek.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Kurt Kanzenbach Nov. 9, 2020, 12:51 p.m. UTC | #1
On Mon Nov 09 2020, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The left shift of u16 variable high is promoted to the type int and
> then sign extended to a 64 bit u64 value.  If the top bit of high is
> set then the upper 32 bits of the result end up being set by the
> sign extension. Fix this by explicitly casting the value in high to
> a u64 before left shifting by 16 places.
>
> Also, remove the initialisation of variable value to 0 at the start
> of each loop iteration as the value is never read and hence the
> assignment it is redundant.
>
> Addresses-Coverity: ("Unintended sign extension")
> Fixes: e4b27ebc780f ("net: dsa: Add DSA driver for Hirschmann Hellcreek switches")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/net/dsa/hirschmann/hellcreek.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
> index dfa66f7260d6..d42f40c76ba5 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -308,7 +308,7 @@ static void hellcreek_get_ethtool_stats(struct dsa_switch *ds, int port,
>  		const struct hellcreek_counter *counter = &hellcreek_counter[i];
>  		u8 offset = counter->offset + port * 64;
>  		u16 high, low;
> -		u64 value = 0;
> +		u64 value;
>  
>  		mutex_lock(&hellcreek->reg_lock);
>  
> @@ -320,7 +320,7 @@ static void hellcreek_get_ethtool_stats(struct dsa_switch *ds, int port,
>  		 */
>  		high  = hellcreek_read(hellcreek, HR_CRDH);
>  		low   = hellcreek_read(hellcreek, HR_CRDL);
> -		value = (high << 16) | low;
> +		value = ((u64)high << 16) | low;

Looks good to me. Thank you.

Thanks,
Kurt
Kurt Kanzenbach Nov. 9, 2020, 1:27 p.m. UTC | #2
On Mon Nov 09 2020, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The left shift of u16 variable high is promoted to the type int and
> then sign extended to a 64 bit u64 value.  If the top bit of high is
> set then the upper 32 bits of the result end up being set by the
> sign extension. Fix this by explicitly casting the value in high to
> a u64 before left shifting by 16 places.
>
> Also, remove the initialisation of variable value to 0 at the start
> of each loop iteration as the value is never read and hence the
> assignment it is redundant.
>
> Addresses-Coverity: ("Unintended sign extension")
> Fixes: e4b27ebc780f ("net: dsa: Add DSA driver for Hirschmann Hellcreek switches")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
Jakub Kicinski Nov. 11, 2020, 1:48 a.m. UTC | #3
On Mon, 09 Nov 2020 14:27:52 +0100 Kurt Kanzenbach wrote:
> On Mon Nov 09 2020, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > The left shift of u16 variable high is promoted to the type int and
> > then sign extended to a 64 bit u64 value.  If the top bit of high is
> > set then the upper 32 bits of the result end up being set by the
> > sign extension. Fix this by explicitly casting the value in high to
> > a u64 before left shifting by 16 places.
> >
> > Also, remove the initialisation of variable value to 0 at the start
> > of each loop iteration as the value is never read and hence the
> > assignment it is redundant.
> >
> > Addresses-Coverity: ("Unintended sign extension")
> > Fixes: e4b27ebc780f ("net: dsa: Add DSA driver for Hirschmann Hellcreek switches")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>  
> 
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>

Applied, thanks!
diff mbox series

Patch

diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index dfa66f7260d6..d42f40c76ba5 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -308,7 +308,7 @@  static void hellcreek_get_ethtool_stats(struct dsa_switch *ds, int port,
 		const struct hellcreek_counter *counter = &hellcreek_counter[i];
 		u8 offset = counter->offset + port * 64;
 		u16 high, low;
-		u64 value = 0;
+		u64 value;
 
 		mutex_lock(&hellcreek->reg_lock);
 
@@ -320,7 +320,7 @@  static void hellcreek_get_ethtool_stats(struct dsa_switch *ds, int port,
 		 */
 		high  = hellcreek_read(hellcreek, HR_CRDH);
 		low   = hellcreek_read(hellcreek, HR_CRDL);
-		value = (high << 16) | low;
+		value = ((u64)high << 16) | low;
 
 		hellcreek_port->counter_values[i] += value;
 		data[i] = hellcreek_port->counter_values[i];