| Message ID | 20220624131858.1064197-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | a2d202bdee194de6196cc1f963e3d289d0b09401 |
| Headers | show |
| Series | [ovs-dev] tc: Fix misaligned access to stats and time values. | 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 6/24/22 15:18, Ilya Maximets wrote: > Pointers to gnet_stats_basic and tcf_t are not correctly aligned, > so we need to copy the data before accessing. Found by running > check-offloads testsuite with UBsan: > > lib/tc.c:1791:50: runtime error: > member access within misaligned address 0x61900005ce1c for type > 'const struct gnet_stats_basic', which requires 8 byte alignment > 0x61900005ce1c: note: pointer points here > 14 00 07 00 00 00 00 00 00 00 00 00 00 00 00 00 ... > ^ > 0 0xd69044 in nl_parse_single_action lib/tc.c:1791:50 > 1 0xd69044 in nl_parse_flower_actions lib/tc.c:1841:19 > 2 0xd57612 in nl_parse_flower_options lib/tc.c:1893:12 > 3 0xd5468d in parse_netlink_to_tc_flower lib/tc.c:1941:12 > 4 0xd5a7ec in tc_replace_flower lib/tc.c:3199:19 > 5 0xd2baea in probe_tc_block_support lib/netdev-offload-tc.c:2226:13 > 6 0xd2baea in netdev_tc_init_flow_api lib/netdev-offload-tc.c:2279:9 > 7 0x94d726 in netdev_assign_flow_api lib/netdev-offload.c:183:14 > 8 0x94d726 in netdev_init_flow_api lib/netdev-offload.c:323:9 > 9 0x9515c7 in netdev_ports_flow_init lib/netdev-offload.c:775:8 > 10 0x9515c7 in netdev_set_flow_api_enabled lib/netdev-offload.c:815:13 > 11 0x562ec8 in bridge_run vswitchd/bridge.c:3292:9 > 12 0x59a98c in main vswitchd/ovs-vswitchd.c:129:9 > 13 0x7fb5c583acf2 in __libc_start_main (/lib64/libc.so.6+0x3acf2) > 14 0x47e60d in _start (vswitchd/ovs-vswitchd+0x47e60d) > > lib/tc.c:1306:43: runtime error: > member access within misaligned address 0x619000140324 for type > 'const struct tcf_t', which requires 8 byte alignment > 0x619000140324: note: pointer points here > 24 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 ... > ^ > 0 0xd6ee6f in nl_parse_tcf lib/tc.c:1306:43 > 1 0xd6423f in nl_parse_act_mirred lib/tc.c:1389:5 > 2 0xd6423f in nl_parse_single_action lib/tc.c:1747:15 > 3 0xd6423f in nl_parse_flower_actions lib/tc.c:1843:19 > 4 0xd57612 in nl_parse_flower_options lib/tc.c:1895:12 > 5 0xd5468d in parse_netlink_to_tc_flower lib/tc.c:1943:12 > 6 0xd5a7ec in tc_replace_flower lib/tc.c:3201:19 > 7 0xd28ae8 in netdev_tc_flow_put lib/netdev-offload-tc.c:1969:11 > 8 0x94cc97 in netdev_flow_put lib/netdev-offload.c:257:14 > 9 0xcba2be in parse_flow_put lib/dpif-netlink.c:2289:11 > 10 0xcba2be in try_send_to_netdev lib/dpif-netlink.c:2376:15 > 11 0xcba2be in dpif_netlink_operate lib/dpif-netlink.c:2447:23 > 12 0x8647ce in dpif_operate lib/dpif.c:1372:13 > 13 0x6b50a6 in push_dp_ops ofproto/ofproto-dpif-upcall.c:2406:5 > 14 0x6c9bcd in revalidate ofproto/ofproto-dpif-upcall.c:2792:13 > 15 0x6b79b5 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:980:9 > 16 0xb4d5ea in ovsthread_wrapper lib/ovs-thread.c:422:12 > 17 0x7ff1090081ce in start_thread (/lib64/libpthread.so.0+0x81ce) > 18 0x7ff107c39dd2 in clone (/lib64/libc.so.6+0x39dd2) > > This patch fixes only problems reported by UBsan in current tests, > there could be more issues like this not currently covered by the > testsuite. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- Looks good to me, thanks! Acked-by: Dumitru Ceara <dceara@redhat.com>
On 24 Jun 2022, at 15:18, Ilya Maximets wrote: > Pointers to gnet_stats_basic and tcf_t are not correctly aligned, > so we need to copy the data before accessing. Found by running > check-offloads testsuite with UBsan: > > lib/tc.c:1791:50: runtime error: > member access within misaligned address 0x61900005ce1c for type > 'const struct gnet_stats_basic', which requires 8 byte alignment > 0x61900005ce1c: note: pointer points here > 14 00 07 00 00 00 00 00 00 00 00 00 00 00 00 00 ... > ^ > 0 0xd69044 in nl_parse_single_action lib/tc.c:1791:50 > 1 0xd69044 in nl_parse_flower_actions lib/tc.c:1841:19 > 2 0xd57612 in nl_parse_flower_options lib/tc.c:1893:12 > 3 0xd5468d in parse_netlink_to_tc_flower lib/tc.c:1941:12 > 4 0xd5a7ec in tc_replace_flower lib/tc.c:3199:19 > 5 0xd2baea in probe_tc_block_support lib/netdev-offload-tc.c:2226:13 > 6 0xd2baea in netdev_tc_init_flow_api lib/netdev-offload-tc.c:2279:9 > 7 0x94d726 in netdev_assign_flow_api lib/netdev-offload.c:183:14 > 8 0x94d726 in netdev_init_flow_api lib/netdev-offload.c:323:9 > 9 0x9515c7 in netdev_ports_flow_init lib/netdev-offload.c:775:8 > 10 0x9515c7 in netdev_set_flow_api_enabled lib/netdev-offload.c:815:13 > 11 0x562ec8 in bridge_run vswitchd/bridge.c:3292:9 > 12 0x59a98c in main vswitchd/ovs-vswitchd.c:129:9 > 13 0x7fb5c583acf2 in __libc_start_main (/lib64/libc.so.6+0x3acf2) > 14 0x47e60d in _start (vswitchd/ovs-vswitchd+0x47e60d) > > lib/tc.c:1306:43: runtime error: > member access within misaligned address 0x619000140324 for type > 'const struct tcf_t', which requires 8 byte alignment > 0x619000140324: note: pointer points here > 24 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 ... > ^ > 0 0xd6ee6f in nl_parse_tcf lib/tc.c:1306:43 > 1 0xd6423f in nl_parse_act_mirred lib/tc.c:1389:5 > 2 0xd6423f in nl_parse_single_action lib/tc.c:1747:15 > 3 0xd6423f in nl_parse_flower_actions lib/tc.c:1843:19 > 4 0xd57612 in nl_parse_flower_options lib/tc.c:1895:12 > 5 0xd5468d in parse_netlink_to_tc_flower lib/tc.c:1943:12 > 6 0xd5a7ec in tc_replace_flower lib/tc.c:3201:19 > 7 0xd28ae8 in netdev_tc_flow_put lib/netdev-offload-tc.c:1969:11 > 8 0x94cc97 in netdev_flow_put lib/netdev-offload.c:257:14 > 9 0xcba2be in parse_flow_put lib/dpif-netlink.c:2289:11 > 10 0xcba2be in try_send_to_netdev lib/dpif-netlink.c:2376:15 > 11 0xcba2be in dpif_netlink_operate lib/dpif-netlink.c:2447:23 > 12 0x8647ce in dpif_operate lib/dpif.c:1372:13 > 13 0x6b50a6 in push_dp_ops ofproto/ofproto-dpif-upcall.c:2406:5 > 14 0x6c9bcd in revalidate ofproto/ofproto-dpif-upcall.c:2792:13 > 15 0x6b79b5 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:980:9 > 16 0xb4d5ea in ovsthread_wrapper lib/ovs-thread.c:422:12 > 17 0x7ff1090081ce in start_thread (/lib64/libpthread.so.0+0x81ce) > 18 0x7ff107c39dd2 in clone (/lib64/libc.so.6+0x39dd2) > > This patch fixes only problems reported by UBsan in current tests, > there could be more issues like this not currently covered by the > testsuite. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Changes look good to me, and tests are passing. Copied in Jianbo Liu, as his “netdev-linux: Add functions to manipulate tc police action” patch will also be affected. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 6/24/22 16:34, Eelco Chaudron wrote: > > > On 24 Jun 2022, at 15:18, Ilya Maximets wrote: > >> Pointers to gnet_stats_basic and tcf_t are not correctly aligned, >> so we need to copy the data before accessing. Found by running >> check-offloads testsuite with UBsan: >> >> lib/tc.c:1791:50: runtime error: >> member access within misaligned address 0x61900005ce1c for type >> 'const struct gnet_stats_basic', which requires 8 byte alignment >> 0x61900005ce1c: note: pointer points here >> 14 00 07 00 00 00 00 00 00 00 00 00 00 00 00 00 ... >> ^ >> 0 0xd69044 in nl_parse_single_action lib/tc.c:1791:50 >> 1 0xd69044 in nl_parse_flower_actions lib/tc.c:1841:19 >> 2 0xd57612 in nl_parse_flower_options lib/tc.c:1893:12 >> 3 0xd5468d in parse_netlink_to_tc_flower lib/tc.c:1941:12 >> 4 0xd5a7ec in tc_replace_flower lib/tc.c:3199:19 >> 5 0xd2baea in probe_tc_block_support lib/netdev-offload-tc.c:2226:13 >> 6 0xd2baea in netdev_tc_init_flow_api lib/netdev-offload-tc.c:2279:9 >> 7 0x94d726 in netdev_assign_flow_api lib/netdev-offload.c:183:14 >> 8 0x94d726 in netdev_init_flow_api lib/netdev-offload.c:323:9 >> 9 0x9515c7 in netdev_ports_flow_init lib/netdev-offload.c:775:8 >> 10 0x9515c7 in netdev_set_flow_api_enabled lib/netdev-offload.c:815:13 >> 11 0x562ec8 in bridge_run vswitchd/bridge.c:3292:9 >> 12 0x59a98c in main vswitchd/ovs-vswitchd.c:129:9 >> 13 0x7fb5c583acf2 in __libc_start_main (/lib64/libc.so.6+0x3acf2) >> 14 0x47e60d in _start (vswitchd/ovs-vswitchd+0x47e60d) >> >> lib/tc.c:1306:43: runtime error: >> member access within misaligned address 0x619000140324 for type >> 'const struct tcf_t', which requires 8 byte alignment >> 0x619000140324: note: pointer points here >> 24 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 ... >> ^ >> 0 0xd6ee6f in nl_parse_tcf lib/tc.c:1306:43 >> 1 0xd6423f in nl_parse_act_mirred lib/tc.c:1389:5 >> 2 0xd6423f in nl_parse_single_action lib/tc.c:1747:15 >> 3 0xd6423f in nl_parse_flower_actions lib/tc.c:1843:19 >> 4 0xd57612 in nl_parse_flower_options lib/tc.c:1895:12 >> 5 0xd5468d in parse_netlink_to_tc_flower lib/tc.c:1943:12 >> 6 0xd5a7ec in tc_replace_flower lib/tc.c:3201:19 >> 7 0xd28ae8 in netdev_tc_flow_put lib/netdev-offload-tc.c:1969:11 >> 8 0x94cc97 in netdev_flow_put lib/netdev-offload.c:257:14 >> 9 0xcba2be in parse_flow_put lib/dpif-netlink.c:2289:11 >> 10 0xcba2be in try_send_to_netdev lib/dpif-netlink.c:2376:15 >> 11 0xcba2be in dpif_netlink_operate lib/dpif-netlink.c:2447:23 >> 12 0x8647ce in dpif_operate lib/dpif.c:1372:13 >> 13 0x6b50a6 in push_dp_ops ofproto/ofproto-dpif-upcall.c:2406:5 >> 14 0x6c9bcd in revalidate ofproto/ofproto-dpif-upcall.c:2792:13 >> 15 0x6b79b5 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:980:9 >> 16 0xb4d5ea in ovsthread_wrapper lib/ovs-thread.c:422:12 >> 17 0x7ff1090081ce in start_thread (/lib64/libpthread.so.0+0x81ce) >> 18 0x7ff107c39dd2 in clone (/lib64/libc.so.6+0x39dd2) >> >> This patch fixes only problems reported by UBsan in current tests, >> there could be more issues like this not currently covered by the >> testsuite. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > > Changes look good to me, and tests are passing. > > Copied in Jianbo Liu, as his “netdev-linux: Add functions to manipulate tc police action” patch will also be affected. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> Thanks, Eelco and Dumitru. Applied to master and branch-2.17. Best regards, Ilya Maximets.
diff --git a/lib/tc.c b/lib/tc.c index df73a43d4..bbb8c86f7 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1312,8 +1312,8 @@ nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower) struct nlattr *gact_attrs[ARRAY_SIZE(gact_policy)]; const struct tc_gact *p; struct nlattr *gact_parms; - const struct tcf_t *tm; struct tc_action *action; + struct tcf_t tm; if (!nl_parse_nested(options, gact_policy, gact_attrs, ARRAY_SIZE(gact_policy))) { @@ -1333,8 +1333,9 @@ nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower) return EINVAL; } - tm = nl_attr_get_unspec(gact_attrs[TCA_GACT_TM], sizeof *tm); - nl_parse_tcf(tm, flower); + memcpy(&tm, nl_attr_get_unspec(gact_attrs[TCA_GACT_TM], sizeof tm), + sizeof tm); + nl_parse_tcf(&tm, flower); return 0; } @@ -1355,9 +1356,9 @@ nl_parse_act_mirred(struct nlattr *options, struct tc_flower *flower) struct nlattr *mirred_attrs[ARRAY_SIZE(mirred_policy)]; const struct tc_mirred *m; const struct nlattr *mirred_parms; - const struct tcf_t *tm; struct nlattr *mirred_tm; struct tc_action *action; + struct tcf_t tm; if (!nl_parse_nested(options, mirred_policy, mirred_attrs, ARRAY_SIZE(mirred_policy))) { @@ -1385,8 +1386,8 @@ nl_parse_act_mirred(struct nlattr *options, struct tc_flower *flower) action->type = TC_ACT_OUTPUT; mirred_tm = mirred_attrs[TCA_MIRRED_TM]; - tm = nl_attr_get_unspec(mirred_tm, sizeof *tm); - nl_parse_tcf(tm, flower); + memcpy(&tm, nl_attr_get_unspec(mirred_tm, sizeof tm), sizeof tm); + nl_parse_tcf(&tm, flower); return 0; } @@ -1725,9 +1726,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)]; struct ovs_flow_stats *stats_sw = &flower->stats_sw; struct ovs_flow_stats *stats_hw = &flower->stats_hw; - const struct gnet_stats_basic *bs_all = NULL; - const struct gnet_stats_basic *bs_hw = NULL; - struct gnet_stats_basic bs_sw = { .packets = 0, .bytes = 0, }; + struct gnet_stats_basic bs_all, bs_hw, bs_sw; int err = 0; if (!nl_parse_nested(action, act_policy, action_attrs, @@ -1783,16 +1782,19 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, return EPROTO; } - bs_all = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs_all); + memcpy(&bs_all, + nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof bs_all), + sizeof bs_all); if (stats_attrs[TCA_STATS_BASIC_HW]) { - bs_hw = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW], - sizeof *bs_hw); + memcpy(&bs_hw, nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW], + sizeof bs_hw), + sizeof bs_hw); - bs_sw.packets = bs_all->packets - bs_hw->packets; - bs_sw.bytes = bs_all->bytes - bs_hw->bytes; + bs_sw.packets = bs_all.packets - bs_hw.packets; + bs_sw.bytes = bs_all.bytes - bs_hw.bytes; } else { - bs_sw.packets = bs_all->packets; - bs_sw.bytes = bs_all->bytes; + bs_sw.packets = bs_all.packets; + bs_sw.bytes = bs_all.bytes; } if (bs_sw.packets > get_32aligned_u64(&stats_sw->n_packets)) { @@ -1800,9 +1802,10 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, put_32aligned_u64(&stats_sw->n_bytes, bs_sw.bytes); } - if (bs_hw && bs_hw->packets > get_32aligned_u64(&stats_hw->n_packets)) { - put_32aligned_u64(&stats_hw->n_packets, bs_hw->packets); - put_32aligned_u64(&stats_hw->n_bytes, bs_hw->bytes); + if (stats_attrs[TCA_STATS_BASIC_HW] + && bs_hw.packets > get_32aligned_u64(&stats_hw->n_packets)) { + put_32aligned_u64(&stats_hw->n_packets, bs_hw.packets); + put_32aligned_u64(&stats_hw->n_bytes, bs_hw.bytes); } return 0;
Pointers to gnet_stats_basic and tcf_t are not correctly aligned, so we need to copy the data before accessing. Found by running check-offloads testsuite with UBsan: lib/tc.c:1791:50: runtime error: member access within misaligned address 0x61900005ce1c for type 'const struct gnet_stats_basic', which requires 8 byte alignment 0x61900005ce1c: note: pointer points here 14 00 07 00 00 00 00 00 00 00 00 00 00 00 00 00 ... ^ 0 0xd69044 in nl_parse_single_action lib/tc.c:1791:50 1 0xd69044 in nl_parse_flower_actions lib/tc.c:1841:19 2 0xd57612 in nl_parse_flower_options lib/tc.c:1893:12 3 0xd5468d in parse_netlink_to_tc_flower lib/tc.c:1941:12 4 0xd5a7ec in tc_replace_flower lib/tc.c:3199:19 5 0xd2baea in probe_tc_block_support lib/netdev-offload-tc.c:2226:13 6 0xd2baea in netdev_tc_init_flow_api lib/netdev-offload-tc.c:2279:9 7 0x94d726 in netdev_assign_flow_api lib/netdev-offload.c:183:14 8 0x94d726 in netdev_init_flow_api lib/netdev-offload.c:323:9 9 0x9515c7 in netdev_ports_flow_init lib/netdev-offload.c:775:8 10 0x9515c7 in netdev_set_flow_api_enabled lib/netdev-offload.c:815:13 11 0x562ec8 in bridge_run vswitchd/bridge.c:3292:9 12 0x59a98c in main vswitchd/ovs-vswitchd.c:129:9 13 0x7fb5c583acf2 in __libc_start_main (/lib64/libc.so.6+0x3acf2) 14 0x47e60d in _start (vswitchd/ovs-vswitchd+0x47e60d) lib/tc.c:1306:43: runtime error: member access within misaligned address 0x619000140324 for type 'const struct tcf_t', which requires 8 byte alignment 0x619000140324: note: pointer points here 24 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 ... ^ 0 0xd6ee6f in nl_parse_tcf lib/tc.c:1306:43 1 0xd6423f in nl_parse_act_mirred lib/tc.c:1389:5 2 0xd6423f in nl_parse_single_action lib/tc.c:1747:15 3 0xd6423f in nl_parse_flower_actions lib/tc.c:1843:19 4 0xd57612 in nl_parse_flower_options lib/tc.c:1895:12 5 0xd5468d in parse_netlink_to_tc_flower lib/tc.c:1943:12 6 0xd5a7ec in tc_replace_flower lib/tc.c:3201:19 7 0xd28ae8 in netdev_tc_flow_put lib/netdev-offload-tc.c:1969:11 8 0x94cc97 in netdev_flow_put lib/netdev-offload.c:257:14 9 0xcba2be in parse_flow_put lib/dpif-netlink.c:2289:11 10 0xcba2be in try_send_to_netdev lib/dpif-netlink.c:2376:15 11 0xcba2be in dpif_netlink_operate lib/dpif-netlink.c:2447:23 12 0x8647ce in dpif_operate lib/dpif.c:1372:13 13 0x6b50a6 in push_dp_ops ofproto/ofproto-dpif-upcall.c:2406:5 14 0x6c9bcd in revalidate ofproto/ofproto-dpif-upcall.c:2792:13 15 0x6b79b5 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:980:9 16 0xb4d5ea in ovsthread_wrapper lib/ovs-thread.c:422:12 17 0x7ff1090081ce in start_thread (/lib64/libpthread.so.0+0x81ce) 18 0x7ff107c39dd2 in clone (/lib64/libc.so.6+0x39dd2) This patch fixes only problems reported by UBsan in current tests, there could be more issues like this not currently covered by the testsuite. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/tc.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)