| Message ID | 20230327203207.514631-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-tc-offloads: Fix misaligned 8 byte read. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 3/27/23 22:32, Mike Pattrick wrote: > UB Sanitizer report: > > lib/netdev-offload-tc.c:1276:19: runtime error: load of misaligned > address 0x7f74e801976c for type 'union ovs_u128', which requires 8 byte > alignment > > #0 in netdev_tc_flow_dump_next lib/netdev-offload-tc.c:1276 > #1 in netdev_flow_dump_next lib/netdev-offload.c:303 > #2 in dpif_netlink_flow_dump_next lib/dpif-netlink.c:1921 > [...] > Thanks for the fix, Mike! How did you catch it? UBsan doesn't report that for me while running a check-offloads testsuite. > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/netdev-offload-tc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 4fb9d9f21..506b74ce7 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -1273,7 +1273,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, > } > > if (flower.act_cookie.len) { > - *ufid = *((ovs_u128 *) flower.act_cookie.data); > + memcpy(ufid, flower.act_cookie.data, sizeof(ovs_u128)); Please, don't use sizeof(type). It's prone to errors and also against the coding style. 'sizeof *ufid' should be used instead. What is the actual alignment of this structure? If it's 4-bytes, then we should use get_32aligned_u128() instead to be more clear on what is going on here. Best regards, Ilya Maximets.
On 27 Mar 2023, at 22:32, Mike Pattrick wrote: > UB Sanitizer report: > > lib/netdev-offload-tc.c:1276:19: runtime error: load of misaligned > address 0x7f74e801976c for type 'union ovs_u128', which requires 8 byte > alignment > > #0 in netdev_tc_flow_dump_next lib/netdev-offload-tc.c:1276 > #1 in netdev_flow_dump_next lib/netdev-offload.c:303 > #2 in dpif_netlink_flow_dump_next lib/dpif-netlink.c:1921 > [...] > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/netdev-offload-tc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 4fb9d9f21..506b74ce7 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -1273,7 +1273,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, > } > > if (flower.act_cookie.len) { The fix looks good to me, but should we maybe also add some minimal size check? If (flower.act_cokkie.len >= sizeof(ovs_u128), or an exact match assuming this really is a ufid? The latter might need some research to make sure we are not returning a longer length due to some padding that could happen, etc. > - *ufid = *((ovs_u128 *) flower.act_cookie.data); > + memcpy(ufid, flower.act_cookie.data, sizeof(ovs_u128)); > } else if (!find_ufid(netdev, &id, ufid)) { > continue; > } > -- > 2.39.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Mar 28, 2023 at 6:02 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 3/27/23 22:32, Mike Pattrick wrote: > > UB Sanitizer report: > > > > lib/netdev-offload-tc.c:1276:19: runtime error: load of misaligned > > address 0x7f74e801976c for type 'union ovs_u128', which requires 8 byte > > alignment > > > > #0 in netdev_tc_flow_dump_next lib/netdev-offload-tc.c:1276 > > #1 in netdev_flow_dump_next lib/netdev-offload.c:303 > > #2 in dpif_netlink_flow_dump_next lib/dpif-netlink.c:1921 > > [...] > > > > Thanks for the fix, Mike! > > How did you catch it? UBsan doesn't report that for me while > running a check-offloads testsuite. I compiled with gcc 11.3.1 (20221121) if that helps. Other than that, with current master: # ./configure CFLAGS="-fsanitize=undefined" # make -j 50 # make check-offloads TESTSUITEFLAGS="2" ## ------------------------------ ## ## openvswitch 3.1.90 test suite. ## ## ------------------------------ ## 2: offloads - ping between two ports - offloads enabled FAILED (system-offloads-traffic.at:72) However, clang version 15.0.7 doesn't identify this spot. > > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > --- > > lib/netdev-offload-tc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > > index 4fb9d9f21..506b74ce7 100644 > > --- a/lib/netdev-offload-tc.c > > +++ b/lib/netdev-offload-tc.c > > @@ -1273,7 +1273,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, > > } > > > > if (flower.act_cookie.len) { > > - *ufid = *((ovs_u128 *) flower.act_cookie.data); > > + memcpy(ufid, flower.act_cookie.data, sizeof(ovs_u128)); > > Please, don't use sizeof(type). It's prone to errors and also > against the coding style. 'sizeof *ufid' should be used instead. > > What is the actual alignment of this structure? If it's 4-bytes, > then we should use get_32aligned_u128() instead to be more clear > on what is going on here. I'll resubmit with this correction and Eelco's excellent suggestion. Cheers, M > > Best regards, Ilya Maximets. >
On 3/28/23 16:21, Mike Pattrick wrote: > On Tue, Mar 28, 2023 at 6:02 AM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 3/27/23 22:32, Mike Pattrick wrote: >>> UB Sanitizer report: >>> >>> lib/netdev-offload-tc.c:1276:19: runtime error: load of misaligned >>> address 0x7f74e801976c for type 'union ovs_u128', which requires 8 byte >>> alignment >>> >>> #0 in netdev_tc_flow_dump_next lib/netdev-offload-tc.c:1276 >>> #1 in netdev_flow_dump_next lib/netdev-offload.c:303 >>> #2 in dpif_netlink_flow_dump_next lib/dpif-netlink.c:1921 >>> [...] >>> >> >> Thanks for the fix, Mike! >> >> How did you catch it? UBsan doesn't report that for me while >> running a check-offloads testsuite. > > I compiled with gcc 11.3.1 (20221121) if that helps. Other than that, > with current master: > > # ./configure CFLAGS="-fsanitize=undefined" > # make -j 50 > # make check-offloads TESTSUITEFLAGS="2" > ## ------------------------------ ## > ## openvswitch 3.1.90 test suite. ## > ## ------------------------------ ## > 2: offloads - ping between two ports - offloads enabled FAILED > (system-offloads-traffic.at:72) > > However, clang version 15.0.7 doesn't identify this spot. Hmm, interesting. I mostly use clang for sanitizers. > >> >>> Signed-off-by: Mike Pattrick <mkp@redhat.com> >>> --- >>> lib/netdev-offload-tc.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>> index 4fb9d9f21..506b74ce7 100644 >>> --- a/lib/netdev-offload-tc.c >>> +++ b/lib/netdev-offload-tc.c >>> @@ -1273,7 +1273,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, >>> } >>> >>> if (flower.act_cookie.len) { >>> - *ufid = *((ovs_u128 *) flower.act_cookie.data); >>> + memcpy(ufid, flower.act_cookie.data, sizeof(ovs_u128)); >> >> Please, don't use sizeof(type). It's prone to errors and also >> against the coding style. 'sizeof *ufid' should be used instead. >> >> What is the actual alignment of this structure? If it's 4-bytes, >> then we should use get_32aligned_u128() instead to be more clear >> on what is going on here. > > I'll resubmit with this correction and Eelco's excellent suggestion. > > > Cheers, > M > >> >> Best regards, Ilya Maximets. >> >
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 4fb9d9f21..506b74ce7 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -1273,7 +1273,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, } if (flower.act_cookie.len) { - *ufid = *((ovs_u128 *) flower.act_cookie.data); + memcpy(ufid, flower.act_cookie.data, sizeof(ovs_u128)); } else if (!find_ufid(netdev, &id, ufid)) { continue; }
UB Sanitizer report: lib/netdev-offload-tc.c:1276:19: runtime error: load of misaligned address 0x7f74e801976c for type 'union ovs_u128', which requires 8 byte alignment #0 in netdev_tc_flow_dump_next lib/netdev-offload-tc.c:1276 #1 in netdev_flow_dump_next lib/netdev-offload.c:303 #2 in dpif_netlink_flow_dump_next lib/dpif-netlink.c:1921 [...] Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/netdev-offload-tc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)