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

Message ID 1520291041-15769-1-git-send-email-ashishvarma.ovs@gmail.com
State Accepted
Headers show
Series
  • [ovs-dev,v1] ofproto-dpif-upcall: fix for segmentation fault
Related show

Commit Message

Ashish Varma March 5, 2018, 11:04 p.m.
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@gmail.com>
---
 ofproto/ofproto-dpif-upcall.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Ben Pfaff March 6, 2018, 12:09 a.m. | #1
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@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.
Justin Pettit March 6, 2018, 12:20 a.m. | #2
> On Mar 5, 2018, at 4:09 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> 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@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?

I actually think it was an oversight of the reviewer.  (J'accuse.)

Yes, I agree that was in error.  Thanks for catching it, guys.

--Justin

Patch

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);