| Message ID | 20201207144503.169679-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [next] seg6: fix unintentional integer overflow on left shift | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, 7 Dec 2020 14:45:03 +0000 Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@canonical.com> > > Shifting the integer value 1 is evaluated using 32-bit arithmetic > and then used in an expression that expects a unsigned long value > leads to a potential integer overflow. Fix this by using the BIT > macro to perform the shift to avoid the overflow. > > Addresses-Coverity: ("Uninitentional integer overflow") > Fixes: 964adce526a4 ("seg6: improve management of behavior attributes") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > net/ipv6/seg6_local.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > index b07f7c1c82a4..d68de8cd1207 100644 > --- a/net/ipv6/seg6_local.c > +++ b/net/ipv6/seg6_local.c > @@ -1366,7 +1366,7 @@ static void __destroy_attrs(unsigned long parsed_attrs, int max_parsed, > * attribute; otherwise, we call the destroy() callback. > */ > for (i = 0; i < max_parsed; ++i) { > - if (!(parsed_attrs & (1 << i))) > + if (!(parsed_attrs & BIT(i))) > continue; > > param = &seg6_action_params[i]; > -- > 2.29.2 > Hi Colin, thanks for the fix. I've just given a look a the whole seg6_local.c code and I found that such issues is present in other parts of the code. If we agree, I can make a fix which explicitly eliminates the several (1 << i) in favor of BIT(i). Andrea
On 07/12/2020 19:59, Andrea Mayer wrote: > On Mon, 7 Dec 2020 14:45:03 +0000 > Colin King <colin.king@canonical.com> wrote: > >> From: Colin Ian King <colin.king@canonical.com> >> >> Shifting the integer value 1 is evaluated using 32-bit arithmetic >> and then used in an expression that expects a unsigned long value >> leads to a potential integer overflow. Fix this by using the BIT >> macro to perform the shift to avoid the overflow. >> >> Addresses-Coverity: ("Uninitentional integer overflow") >> Fixes: 964adce526a4 ("seg6: improve management of behavior attributes") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> net/ipv6/seg6_local.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c >> index b07f7c1c82a4..d68de8cd1207 100644 >> --- a/net/ipv6/seg6_local.c >> +++ b/net/ipv6/seg6_local.c >> @@ -1366,7 +1366,7 @@ static void __destroy_attrs(unsigned long parsed_attrs, int max_parsed, >> * attribute; otherwise, we call the destroy() callback. >> */ >> for (i = 0; i < max_parsed; ++i) { >> - if (!(parsed_attrs & (1 << i))) >> + if (!(parsed_attrs & BIT(i))) >> continue; >> >> param = &seg6_action_params[i]; >> -- >> 2.29.2 >> > > Hi Colin, > thanks for the fix. I've just given a look a the whole seg6_local.c code and I > found that such issues is present in other parts of the code. > > If we agree, I can make a fix which explicitly eliminates the several (1 << i) > in favor of BIT(i). Sounds good to me. Colin > > Andrea >
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c index b07f7c1c82a4..d68de8cd1207 100644 --- a/net/ipv6/seg6_local.c +++ b/net/ipv6/seg6_local.c @@ -1366,7 +1366,7 @@ static void __destroy_attrs(unsigned long parsed_attrs, int max_parsed, * attribute; otherwise, we call the destroy() callback. */ for (i = 0; i < max_parsed; ++i) { - if (!(parsed_attrs & (1 << i))) + if (!(parsed_attrs & BIT(i))) continue; param = &seg6_action_params[i];