[ovs-dev] [PATCH] ofp-ed-props: Fix using uninitialized padding for NSH encap actions.

[Jan Scheurich]

> >> Fix that by clearing padding bytes while encoding, and checking that
> >> these bytes are all zeros on decoding.
> >
> > Is the latter strictly necessary? It may break existing controllers that do not
> initialize the padding bytes to zero.
> > Wouldn't it be sufficient to just zero the padding bytes at reception?
> 
> I do not have a strong opinion.  I guess, we could not fail OF request if
> padding is not all zeroes for backward compatibility.
> Anyway, it seems like I missed one part of this change (see inline).
> 
> On the other hand, AFAIU, NXOXM_NSH_ is not standardized, so, technically,
> we could change the rules here.  As an option, we could apply the patch
> without checking for all-zeroes padding and backport it this way to stable
> branches.  Afterwards, we could introduce the 'is_all_zeros' check and
> mention this change in release notes for the new version.  Anyway OpenFlow
> usually requires paddings to be all-zeroes for most of matches and actions, so
> this should be a sane requirement for controllers.
> What do you think?
> 

I think there is little to gain by enforcing strict rules on zeroed padding bytes in a future release. It just creates grief with users of OVS by unnecessarily breaking backward compatibility without any benefit for OVS. No matter if OVS is has the right to do so or not.

> >> diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index
> >> 28382e012..5a4b12d9f 100644
> >> --- a/lib/ofp-ed-props.c
> >> +++ b/lib/ofp-ed-props.c
> >> @@ -48,6 +48,9 @@ decode_ed_prop(const struct ofp_ed_prop_header
> >> **ofp_prop,
> >>              if (len > sizeof(*opnmt) || len > *remaining) {
> >>                  return OFPERR_NXBAC_BAD_ED_PROP;
> >>              }
> >> +            if (!is_all_zeros(opnmt->pad, sizeof opnmt->pad)) {
> >> +                return OFPERR_NXBRC_MUST_BE_ZERO;
> >> +            }
> >>              struct ofpact_ed_prop_nsh_md_type *pnmt =
> >>                      ofpbuf_put_uninit(out, sizeof(*pnmt));
> 
> This should be 'ofpbuf_put_zeroes' because 'struct
> ofpact_ed_prop_nsh_md_type'
> contains padding too that must be cleared while constructing ofpacts.
> Since OVS compares decoded ofpacts' and not the original OF messages, this
> should do the trick.

Agree.

> 
> I'll send v2 with this change and will remove 'is_all_zeros' check for this fix.

Thanks, Jan