[ovs-dev] [PATCH v1] ofproto-dpif-upcall: fix for segmentation fault

[Ben Pfaff]

On Mon, Mar 05, 2018 at 03:04:01PM -0800, Ashish Varma wrote:
> Added check for NULL pointer on return from xlate_lookup_ofproto
> function. Access to "ofproto" variable when NULL was causing segmentation
> fault.
> 
> VMware-BZ: #2061914
> Signed-off-by: Ashish Varma <ashishvarma.ovs at gmail.com>
> ---
>  ofproto/ofproto-dpif-upcall.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 526be77..0079674 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2129,6 +2129,11 @@ revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey,
>          ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port);
>  
>          ofpbuf_clear(odp_actions);
> +
> +        if (!ofproto) {
> +            goto exit;
> +        }
> +
>          compose_slow_path(udpif, xoutp, &ctx.flow, ctx.flow.in_port.odp_port,
>                            ofp_in_port, odp_actions,
>                            ofproto->up.slowpath_meter_id, &ofproto->uuid);

This bug got introduced in commit d39ec23de384 (ofproto-dpif: Don't
slow-path controller actions.), which introduced the following change.
The change seems to conscientiously decide that 'ofproto' could no
longer be NULL, since it removes null tests.  Justin, do you think you
just overlooked the possibility of null?  Ashish's commit will obviously
fix the crash; do you think that it is the correct change?

@@ -2040,19 +2102,16 @@ revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey,
 
     if (xoutp->slow) {
         struct ofproto_dpif *ofproto;
         ofp_port_t ofp_in_port;
 
-        ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow,
-                                       &ofp_in_port);
-        uint32_t smid = ofproto ? ofproto->up.slowpath_meter_id : UINT32_MAX;
-        uint32_t cmid = ofproto ? ofproto->up.controller_meter_id : UINT32_MAX;
+        ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port);
 
         ofpbuf_clear(odp_actions);
         compose_slow_path(udpif, xoutp, &ctx.flow, ctx.flow.in_port.odp_port,
-                          ofp_in_port, odp_actions, smid, cmid,
-                          &ofproto->uuid);
+                          ofp_in_port, odp_actions,
+                          ofproto->up.slowpath_meter_id, &ofproto->uuid);
     }
 
     if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, &dp_mask, &ctx.flow)
         == ODP_FIT_ERROR) {
         goto exit;

Thanks,

Ben.