[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