| Message ID | 20240517183342.132471-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 320f7e1a408e9956b9bc2144b3886eaf5795090e |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] srv6: Fix misaligned writes to segment list. | 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 2024/05/18 3:33, Ilya Maximets wrote: > Segments list in SRv6 header is 16-bit aligned as most of other fields > in packet headers. A little counter-intuitively, compilers are allowed > to make alignment assumptions based on the pointer type passed to > memcpy(), so they can use copy instructions that require 32-bit alignment > in case of struct in6_addr pointer. Reported by UBsan in Clang 18: > > lib/netdev-native-tnl.c:985:16: runtime error: store to misaligned > address 0x7fd9e97351ce for type 'struct in6_addr *', which > requires 4 byte alignment > 0x7fd9e97351ce: note: pointer points here > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ^ > 0 0xc1de38 in netdev_srv6_build_header lib/netdev-native-tnl.c:985:9 > 1 0x6e794b in tnl_port_build_header ofproto/tunnel.c:751:11 > 2 0x6c9c0a in native_tunnel_output ofproto/ofproto-dpif-xlate.c:3887:11 > 3 0x6c9c0a in compose_output_action__ ofproto/ofproto-dpif-xlate.c:4502:13 > 4 0x6b6646 in compose_output_action ofproto/ofproto-dpif-xlate.c:4564:5 > 5 0x6b6646 in xlate_output_action ofproto/ofproto-dpif-xlate.c:5517:13 > 6 0x68cfee in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7288:13 > 7 0x67fed0 in xlate_actions ofproto/ofproto-dpif-xlate.c:8314:13 > 8 0x6468bd in ofproto_trace__ ofproto/ofproto-dpif-trace.c:782:30 > 9 0x64484a in ofproto_trace ofproto/ofproto-dpif-trace.c:851:5 > 10 0x647469 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:490:9 > 11 0xc33771 in process_command lib/unixctl.c:310:13 > 12 0xc33771 in run_connection lib/unixctl.c:344:17 > 13 0xc33771 in unixctl_server_run lib/unixctl.c:395:21 > 14 0x53e6ef in main vswitchd/ovs-vswitchd.c:131:9 > 15 0x7f61c7 in __libc_start_call_main (/lib64/libc.so.6+0x2a1c7) > 16 0x7f628a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a28a) > 17 0x42ca24 in _start (vswitchd/ovs-vswitchd+0x42ca24) > > SUMMARY: UndefinedBehaviorSanitizer: > undefined-behavior lib/netdev-native-tnl.c:985:16 > > Having misaligned pointers is also generally not allowed in C, let > alone accessing memory through them. > > Fix that by using an appropriate ovs_16aligned_in6_addr pointer instead. > > Fixes: 7381fd440a88 ("odp: Add SRv6 tunnel actions.") > Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/netdev-native-tnl.c | 5 ++--- > lib/odp-util.c | 4 ++-- > 2 files changed, 4 insertions(+), 5 deletions(-) Hi Ilya, Thanks for the fix! Reviewed-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> Best Regards, Nobuhiro MIKI
On 17 May 2024, at 20:33, Ilya Maximets wrote: > Segments list in SRv6 header is 16-bit aligned as most of other fields > in packet headers. A little counter-intuitively, compilers are allowed > to make alignment assumptions based on the pointer type passed to > memcpy(), so they can use copy instructions that require 32-bit alignment > in case of struct in6_addr pointer. Reported by UBsan in Clang 18: > > lib/netdev-native-tnl.c:985:16: runtime error: store to misaligned > address 0x7fd9e97351ce for type 'struct in6_addr *', which > requires 4 byte alignment > 0x7fd9e97351ce: note: pointer points here > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ^ > 0 0xc1de38 in netdev_srv6_build_header lib/netdev-native-tnl.c:985:9 > 1 0x6e794b in tnl_port_build_header ofproto/tunnel.c:751:11 > 2 0x6c9c0a in native_tunnel_output ofproto/ofproto-dpif-xlate.c:3887:11 > 3 0x6c9c0a in compose_output_action__ ofproto/ofproto-dpif-xlate.c:4502:13 > 4 0x6b6646 in compose_output_action ofproto/ofproto-dpif-xlate.c:4564:5 > 5 0x6b6646 in xlate_output_action ofproto/ofproto-dpif-xlate.c:5517:13 > 6 0x68cfee in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7288:13 > 7 0x67fed0 in xlate_actions ofproto/ofproto-dpif-xlate.c:8314:13 > 8 0x6468bd in ofproto_trace__ ofproto/ofproto-dpif-trace.c:782:30 > 9 0x64484a in ofproto_trace ofproto/ofproto-dpif-trace.c:851:5 > 10 0x647469 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:490:9 > 11 0xc33771 in process_command lib/unixctl.c:310:13 > 12 0xc33771 in run_connection lib/unixctl.c:344:17 > 13 0xc33771 in unixctl_server_run lib/unixctl.c:395:21 > 14 0x53e6ef in main vswitchd/ovs-vswitchd.c:131:9 > 15 0x7f61c7 in __libc_start_call_main (/lib64/libc.so.6+0x2a1c7) > 16 0x7f628a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a28a) > 17 0x42ca24 in _start (vswitchd/ovs-vswitchd+0x42ca24) > > SUMMARY: UndefinedBehaviorSanitizer: > undefined-behavior lib/netdev-native-tnl.c:985:16 > > Having misaligned pointers is also generally not allowed in C, let > alone accessing memory through them. > > Fix that by using an appropriate ovs_16aligned_in6_addr pointer instead. > > Fixes: 7381fd440a88 ("odp: Add SRv6 tunnel actions.") > Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Thanks for fixing this, looks good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 5/20/24 03:54, Nobuhiro MIKI wrote: > On 2024/05/18 3:33, Ilya Maximets wrote: >> Segments list in SRv6 header is 16-bit aligned as most of other fields >> in packet headers. A little counter-intuitively, compilers are allowed >> to make alignment assumptions based on the pointer type passed to >> memcpy(), so they can use copy instructions that require 32-bit alignment >> in case of struct in6_addr pointer. Reported by UBsan in Clang 18: >> >> lib/netdev-native-tnl.c:985:16: runtime error: store to misaligned >> address 0x7fd9e97351ce for type 'struct in6_addr *', which >> requires 4 byte alignment >> 0x7fd9e97351ce: note: pointer points here >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> ^ >> 0 0xc1de38 in netdev_srv6_build_header lib/netdev-native-tnl.c:985:9 >> 1 0x6e794b in tnl_port_build_header ofproto/tunnel.c:751:11 >> 2 0x6c9c0a in native_tunnel_output ofproto/ofproto-dpif-xlate.c:3887:11 >> 3 0x6c9c0a in compose_output_action__ ofproto/ofproto-dpif-xlate.c:4502:13 >> 4 0x6b6646 in compose_output_action ofproto/ofproto-dpif-xlate.c:4564:5 >> 5 0x6b6646 in xlate_output_action ofproto/ofproto-dpif-xlate.c:5517:13 >> 6 0x68cfee in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7288:13 >> 7 0x67fed0 in xlate_actions ofproto/ofproto-dpif-xlate.c:8314:13 >> 8 0x6468bd in ofproto_trace__ ofproto/ofproto-dpif-trace.c:782:30 >> 9 0x64484a in ofproto_trace ofproto/ofproto-dpif-trace.c:851:5 >> 10 0x647469 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:490:9 >> 11 0xc33771 in process_command lib/unixctl.c:310:13 >> 12 0xc33771 in run_connection lib/unixctl.c:344:17 >> 13 0xc33771 in unixctl_server_run lib/unixctl.c:395:21 >> 14 0x53e6ef in main vswitchd/ovs-vswitchd.c:131:9 >> 15 0x7f61c7 in __libc_start_call_main (/lib64/libc.so.6+0x2a1c7) >> 16 0x7f628a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a28a) >> 17 0x42ca24 in _start (vswitchd/ovs-vswitchd+0x42ca24) >> >> SUMMARY: UndefinedBehaviorSanitizer: >> undefined-behavior lib/netdev-native-tnl.c:985:16 >> >> Having misaligned pointers is also generally not allowed in C, let >> alone accessing memory through them. >> >> Fix that by using an appropriate ovs_16aligned_in6_addr pointer instead. >> >> Fixes: 7381fd440a88 ("odp: Add SRv6 tunnel actions.") >> Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> lib/netdev-native-tnl.c | 5 ++--- >> lib/odp-util.c | 4 ++-- >> 2 files changed, 4 insertions(+), 5 deletions(-) > > Hi Ilya, > > Thanks for the fix! > > Reviewed-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> Thanks! Applied and backported down to 3.2. Best regards, Ilya Maximets.
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index dee9ab344..b21176037 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -932,9 +932,9 @@ netdev_srv6_build_header(const struct netdev *netdev, const struct netdev_tnl_build_header_params *params) { const struct netdev_tunnel_config *tnl_cfg; + union ovs_16aligned_in6_addr *s; const struct in6_addr *segs; struct srv6_base_hdr *srh; - struct in6_addr *s; ovs_be16 dl_type; int nr_segs; int i; @@ -978,8 +978,7 @@ netdev_srv6_build_header(const struct netdev *netdev, return EOPNOTSUPP; } - s = ALIGNED_CAST(struct in6_addr *, - (char *) srh + sizeof *srh); + s = (union ovs_16aligned_in6_addr *) (srh + 1); for (i = 0; i < nr_segs; i++) { /* Segment list is written to the header in reverse order. */ memcpy(s, &segs[nr_segs - i - 1], sizeof *s); diff --git a/lib/odp-util.c b/lib/odp-util.c index 21f34d955..724e6f2bc 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -1820,8 +1820,8 @@ ovs_parse_tnl_push(const char *s, struct ovs_action_push_tnl *data) } else if (ovs_scan_len(s, &n, "srv6(segments_left=%"SCNu8, &segments_left)) { struct srv6_base_hdr *srh = (struct srv6_base_hdr *) (ip6 + 1); + union ovs_16aligned_in6_addr *segs; char seg_s[IPV6_SCAN_LEN + 1]; - struct in6_addr *segs; struct in6_addr seg; uint8_t n_segs = 0; @@ -1844,7 +1844,7 @@ ovs_parse_tnl_push(const char *s, struct ovs_action_push_tnl *data) return -EINVAL; } - segs = ALIGNED_CAST(struct in6_addr *, srh + 1); + segs = (union ovs_16aligned_in6_addr *) (srh + 1); segs += segments_left; while (ovs_scan_len(s, &n, IPV6_SCAN_FMT, seg_s)
Segments list in SRv6 header is 16-bit aligned as most of other fields in packet headers. A little counter-intuitively, compilers are allowed to make alignment assumptions based on the pointer type passed to memcpy(), so they can use copy instructions that require 32-bit alignment in case of struct in6_addr pointer. Reported by UBsan in Clang 18: lib/netdev-native-tnl.c:985:16: runtime error: store to misaligned address 0x7fd9e97351ce for type 'struct in6_addr *', which requires 4 byte alignment 0x7fd9e97351ce: note: pointer points here 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ 0 0xc1de38 in netdev_srv6_build_header lib/netdev-native-tnl.c:985:9 1 0x6e794b in tnl_port_build_header ofproto/tunnel.c:751:11 2 0x6c9c0a in native_tunnel_output ofproto/ofproto-dpif-xlate.c:3887:11 3 0x6c9c0a in compose_output_action__ ofproto/ofproto-dpif-xlate.c:4502:13 4 0x6b6646 in compose_output_action ofproto/ofproto-dpif-xlate.c:4564:5 5 0x6b6646 in xlate_output_action ofproto/ofproto-dpif-xlate.c:5517:13 6 0x68cfee in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7288:13 7 0x67fed0 in xlate_actions ofproto/ofproto-dpif-xlate.c:8314:13 8 0x6468bd in ofproto_trace__ ofproto/ofproto-dpif-trace.c:782:30 9 0x64484a in ofproto_trace ofproto/ofproto-dpif-trace.c:851:5 10 0x647469 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:490:9 11 0xc33771 in process_command lib/unixctl.c:310:13 12 0xc33771 in run_connection lib/unixctl.c:344:17 13 0xc33771 in unixctl_server_run lib/unixctl.c:395:21 14 0x53e6ef in main vswitchd/ovs-vswitchd.c:131:9 15 0x7f61c7 in __libc_start_call_main (/lib64/libc.so.6+0x2a1c7) 16 0x7f628a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a28a) 17 0x42ca24 in _start (vswitchd/ovs-vswitchd+0x42ca24) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/netdev-native-tnl.c:985:16 Having misaligned pointers is also generally not allowed in C, let alone accessing memory through them. Fix that by using an appropriate ovs_16aligned_in6_addr pointer instead. Fixes: 7381fd440a88 ("odp: Add SRv6 tunnel actions.") Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/netdev-native-tnl.c | 5 ++--- lib/odp-util.c | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-)