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