[ovs-dev] [PATCH 4/4] ofp-actions: Re-fix error path for parsing OpenFlow actions.

Yifeng Sun pkusunyifeng at gmail.com
Thu Aug 30 18:25:07 UTC 2018


Looks good to me, thanks.

Tested-by: Yifeng Sun <pkusunyifeng at gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng at gmail.com>

On Fri, Aug 24, 2018 at 2:51 PM Ben Pfaff <blp at ovn.org> wrote:

> A previous commit attempted to fix the error path when the actions nested
> within clone provoked an error.  However, this commit just introduced a new
> problem in another case, since it made ofpacts_pull_openflow_actions__()
> restore a previously valid pointer to data that might have been
> reallocated.
>
> This commit takes another approach.  Instead of trying to restore anything
> at all, it just defines ofpacts_pull_openflow_actions__() to clear the
> output buffer when there's an error.  It seems that this is less error
> prone.  Most of the callers don't care; this commit fixes up the ones that
> do.
>
> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9975
> Fixes: 20cdd1dbd546 ("ofp-actions: Avoid assertion failure for
> clone(ct(...bad actions...)).")
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>  lib/ofp-actions.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 6adb55d23b02..a80a4a308dba 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -5888,6 +5888,9 @@ decode_NXAST_RAW_CLONE(const struct
> ext_action_header *eah,
>                                              ofp_version,
>                                              1u <<
> OVSINST_OFPIT11_APPLY_ACTIONS,
>                                              out, 0, vl_mff_map,
> tlv_bitmap);
> +    if (error) {
> +        return error;
> +    }
>      clone = ofpbuf_push_uninit(out, sizeof *clone);
>      out->header = &clone->ofpact;
>      ofpact_finish_CLONE(out, &clone);
> @@ -6479,7 +6482,7 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack
> *nac,
>                                              out, OFPACT_CT, vl_mff_map,
>                                              tlv_bitmap);
>      if (error) {
> -        goto out;
> +        return error;
>      }
>
>      conntrack = ofpbuf_push_uninit(out, sizeof(*conntrack));
> @@ -7500,8 +7503,6 @@ ofpacts_pull_openflow_actions__(struct ofpbuf
> *openflow,
>                                  uint64_t *ofpacts_tlv_bitmap)
>  {
>      const struct ofp_action_header *actions;
> -    void *orig_data = ofpacts->data;
> -    size_t orig_size = ofpacts->size;
>      enum ofperr error;
>
>      if (actions_len % OFP_ACTION_ALIGN != 0) {
> @@ -7525,21 +7526,15 @@ ofpacts_pull_openflow_actions__(struct ofpbuf
> *openflow,
>                                 outer_action);
>      }
>      if (error) {
> -        /* Back out changes to 'ofpacts'.  (Normally, decoding would only
> -         * append to 'ofpacts', so that one would expect only to need to
> -         * restore 'ofpacts->size', but some action parsing temporarily
> pulls
> -         * off data from the start of 'ofpacts' and may not properly
> re-push it
> -         * on error paths.) */
> -        ofpacts->data = orig_data;
> -        ofpacts->size = orig_size;
> +        ofpbuf_clear(ofpacts);
>      }
>      return error;
>  }
>
>  /* Attempts to convert 'actions_len' bytes of OpenFlow actions from the
> front
>   * of 'openflow' into ofpacts.  On success, appends the converted actions
> to
> - * 'ofpacts'; on failure, 'ofpacts' is unchanged (but might be
> reallocated) .
> - * Returns 0 if successful, otherwise an OpenFlow error.
> + * 'ofpacts'; on failure, clears 'ofpacts'.  Returns 0 if successful,
> otherwise
> + * an OpenFlow error.
>   *
>   * Actions are processed according to their OpenFlow version which
>   * is provided in the 'version' parameter.
> --
> 2.16.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list