On Fri, Jun 14, 2024 at 8:55 AM Ilya Maximets <[email protected]> wrote:
>
> On 6/12/24 20:12, Mike Pattrick wrote:
> > On Wed, Jun 12, 2024 at 9:50 AM Ales Musil <[email protected]> wrote:
> >
> >>
> >>
> >> On Wed, Jun 12, 2024 at 3:32 PM Mike Pattrick <[email protected]> wrote:
> >>
> >>> When compiling with '-fsanitize=address,undefined', the "ovs-ofctl
> >>> ct-flush" test will yield the following undefined behavior flagged
> >>> by UBSan. This patch uses memcpy to move the 128bit value into the
> >>> stack before reading it.
> >>>
> >>> lib/ofp-prop.c:277:14: runtime error: load of misaligned address
> >>> for type 'union ovs_be128', which requires 8 byte alignment
> >>> ^
> >>> #0 0x7735d4 in ofpprop_parse_u128 lib/ofp-prop.c:277
> >>> #1 0x6c6c83 in ofp_ct_match_decode lib/ofp-ct.c:529
> >>> #2 0x76f3b5 in ofp_print_nxt_ct_flush lib/ofp-print.c:959
> >>> #3 0x76f3b5 in ofp_to_string__ lib/ofp-print.c:1206
> >>> #4 0x76f3b5 in ofp_to_string lib/ofp-print.c:1264
> >>> #5 0x770c0d in ofp_print lib/ofp-print.c:1308
> >>> #6 0x484a9d in ofctl_ofp_print utilities/ovs-ofctl.c:4899
> >>> #7 0x4ddb77 in ovs_cmdl_run_command__ lib/command-line.c:247
> >>> #8 0x47f6b3 in main utilities/ovs-ofctl.c:186
>
> Thanks for cleaning up the trace. Please, also remove the '#' tags.
> GitHub trats them as references to issues/PRs and that is annoying.
>
> Also, while most of the addresses in the trace are not important and it's
> good to strip or shorten them, the actual address where the memory access
> happened is important here, so we can see what the actual alignment was.
> Was it part of the original error message? Clang usually provides them,
> not sure about gcc.
>
> >>>
> >>> Signed-off-by: Mike Pattrick <[email protected]>
> >>> ---
> >>>
> >>
> >> Hi Mike,
> >>
> >> this is interesting, do you have an idea why it didn't fail in CI by now?
> >> Also AFAIR the ofprops is supposed to be aligned to 8 bytes so unless the
> >> buffer itself isn't allocated at an address that is not itself 8 bytes
> >> aligned it shouldn't happen. In that case we might actually have a problem
> >> with other sizes.
> >>
> >
> > Report is seen with gcc + ubsan, but not clang + ubsan. It is possible that
> > this is only seen due the test, this warning wasn't seen live.
>
> I agree with Ales on this one. Properties supposed to be aligned.
> We need to find why they are not. i.e. is it a property itself or
> something before it.
I took a closer look at this issue. I first ran into it with
"ovs-ofctl ofp-print", but the same error pops up in ovs-vswitchd with
a command like "ovs-ofctl ct-flush br0 labels=1".
The problem is that the put/parse_u128 functions don't add padding
between the 4 byte header and the data. This is something already
taken care of in the 64bit functions.
I'll resubmit with a more correct solution.
Thanks,
Mike
>
> We may need to take similar approach as in commit:
> a5cc859a4228 ("ofp-actions: Use aligned structures when decoding ofp
> actions.")
>
> >
> > Cheers,
> > M
> >
> >
> >>
> >>
> >>> lib/ofp-prop.c | 4 +++-
> >>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/ofp-prop.c b/lib/ofp-prop.c
> >>> index 0a685750c..ed6365414 100644
> >>> --- a/lib/ofp-prop.c
> >>> +++ b/lib/ofp-prop.c
> >>> @@ -271,10 +271,12 @@ enum ofperr
> >>> ofpprop_parse_u128(const struct ofpbuf *property, ovs_u128 *value)
> >>> {
> >>> ovs_be128 *p = property->msg;
> >>> + ovs_be128 aligned;
> >>> if (ofpbuf_msgsize(property) != sizeof *p) {
> >>> return OFPERR_OFPBPC_BAD_LEN;
> >>> }
> >>> - *value = ntoh128(*p);
> >>> + memcpy(&aligned, p, sizeof aligned);
>
> FWIW, this doesn't actually fix the issue. At least not in all the
> cases. Compiler is free to make alignment assumptions based on the
> pointer type, so we can still have unaligned access inside the memcpy.
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
