[quagga-dev,15441] zebra: make fpm netlink_route_info_fill more robust

Message ID 1465243470-1979-1-git-send-email-chris@opensourcerouting.org
State Under Review
Headers show

Commit Message

Christian Franke June 6, 2016, 8:04 p.m.
From: Christian Franke <nobody@nowhere.ws>

Having an RTM_ADDROUTE with a rib == NULL would lead
to a crash due to a NULL pointer dereference.

Since an RTM_ADDROUTE without a rib object doesn't make
much sense, print a warning and remove the concerned
route instead.

Signed-off-by: Christian Franke <chris@opensourcerouting.org>
---
 zebra/zebra_fpm_netlink.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

cisystem@netdef.org June 6, 2016, 9:20 p.m. | #1
Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF CI System <cisystem@netdef.org>

This is an EXPERIMENTAL automated CI system.
For questions and feedback, feel free to email
Martin Winter <mwinter@opensourcerouting.org>.

Patches applied :
  Patchwork 1958: http://patchwork.quagga.net/patch/1958
       [quagga-dev,15441] zebra: make fpm netlink_route_info_fill more robust
Tested on top of Git : 86c5d2e (as of 20160315.231717 UTC)
CI System Testrun URL: https://ci1.netdef.org/browse/QUAGGA-QPWORK-299/


Regards,
  NetDEF/OpenSourceRouting Continous Integration (CI) System

---
OpenSourceRouting.org is a project of the Network Device Education Foundation,
For more information, see www.netdef.org and www.opensourcerouting.org
For questions in regards to this CI System, contact Martin Winter, mwinter@netdef.org
Donald Sharp June 7, 2016, 2:10 a.m. | #2
Christian -

How is this possible?  In zfpm_encode_route we set cmd == RTM_DELROUTE if
rib == NULL.

Is this setup for some new code?

donald

On Mon, Jun 6, 2016 at 4:04 PM, Christian Franke <
chris@opensourcerouting.org> wrote:

> From: Christian Franke <nobody@nowhere.ws>
>
> Having an RTM_ADDROUTE with a rib == NULL would lead
> to a crash due to a NULL pointer dereference.
>
> Since an RTM_ADDROUTE without a rib object doesn't make
> much sense, print a warning and remove the concerned
> route instead.
>
> Signed-off-by: Christian Franke <chris@opensourcerouting.org>
> ---
>  zebra/zebra_fpm_netlink.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/zebra/zebra_fpm_netlink.c b/zebra/zebra_fpm_netlink.c
> index 0173000..e2c6b8b 100644
> --- a/zebra/zebra_fpm_netlink.c
> +++ b/zebra/zebra_fpm_netlink.c
> @@ -251,11 +251,16 @@ netlink_route_info_fill (netlink_route_info_t *ri,
> int cmd,
>     * An RTM_DELROUTE need not be accompanied by any nexthops,
>     * particularly in our communication with the FPM.
>     */
> -  if (cmd == RTM_DELROUTE && !rib)
> +  if (cmd == RTM_DELROUTE)
>      goto skip;
>
> -  if (rib)
> -    ri->rtm_protocol = netlink_proto_from_route_type (rib->type);
> +  if (!rib)
> +    {
> +      zlog_err("netlink_route_info_fill RTM_ADDROUTE called without rib
> info");
> +      return 0;
> +    }
> +
> +  ri->rtm_protocol = netlink_proto_from_route_type (rib->type);
>
>    if ((rib->flags & ZEBRA_FLAG_BLACKHOLE) || (rib->flags &
> ZEBRA_FLAG_REJECT))
>      discard = 1;
> --
> 2.8.0
>
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev@lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>
Christian Franke June 7, 2016, 5:26 p.m. | #3
On 06/07/2016 04:10 AM, Donald Sharp wrote:
> Christian -
> 
> How is this possible?  In zfpm_encode_route we set cmd == RTM_DELROUTE
> if rib == NULL.

Yes you are right, this seems not to be an issue in practice, I misread
what zfpm_encode_route does, yesterday.

The change came up from an issue found by coverity that boils down to
this check that I removed:

>     -  if (rib)
>     -    ri->rtm_protocol = netlink_proto_from_route_type (rib->type);

Coverity infers from the existance of the check that rib might be NULL
at that point and complains about rib being dereferenced later on,
without any guard for NULL:

>        if ((rib->flags & ZEBRA_FLAG_BLACKHOLE) || (rib->flags &
>     ZEBRA_FLAG_REJECT))
>          discard = 1;

Given that we never have rib == NULL and cmd != RTM_DELROUTE the way
that it's currently called, I see the following options:

a) Leave it as is and mark the coverity issue as non-problematic and
   live with the code that is a bit misleading to read
b) Remove the if (rib) guard
c) Remove it and add an assert(rib != NULL)

I am fine with either and would tend to go with c), but I am open to
suggestions.

-Christian
Donald Sharp June 7, 2016, 11:58 p.m. | #4
I suspected this was a Static Analysis change.  I'd prefer (b), but don't
care enough really to complain if I got a (c) as well.

donald

On Tue, Jun 7, 2016 at 1:26 PM, Christian Franke <
chris@opensourcerouting.org> wrote:

> On 06/07/2016 04:10 AM, Donald Sharp wrote:
> > Christian -
> >
> > How is this possible?  In zfpm_encode_route we set cmd == RTM_DELROUTE
> > if rib == NULL.
>
> Yes you are right, this seems not to be an issue in practice, I misread
> what zfpm_encode_route does, yesterday.
>
> The change came up from an issue found by coverity that boils down to
> this check that I removed:
>
> >     -  if (rib)
> >     -    ri->rtm_protocol = netlink_proto_from_route_type (rib->type);
>
> Coverity infers from the existance of the check that rib might be NULL
> at that point and complains about rib being dereferenced later on,
> without any guard for NULL:
>
> >        if ((rib->flags & ZEBRA_FLAG_BLACKHOLE) || (rib->flags &
> >     ZEBRA_FLAG_REJECT))
> >          discard = 1;
>
> Given that we never have rib == NULL and cmd != RTM_DELROUTE the way
> that it's currently called, I see the following options:
>
> a) Leave it as is and mark the coverity issue as non-problematic and
>    live with the code that is a bit misleading to read
> b) Remove the if (rib) guard
> c) Remove it and add an assert(rib != NULL)
>
> I am fine with either and would tend to go with c), but I am open to
> suggestions.
>
> -Christian
>
>

Patch hide | download patch | download mbox

diff --git a/zebra/zebra_fpm_netlink.c b/zebra/zebra_fpm_netlink.c
index 0173000..e2c6b8b 100644
--- a/zebra/zebra_fpm_netlink.c
+++ b/zebra/zebra_fpm_netlink.c
@@ -251,11 +251,16 @@  netlink_route_info_fill (netlink_route_info_t *ri, int cmd,
    * An RTM_DELROUTE need not be accompanied by any nexthops,
    * particularly in our communication with the FPM.
    */
-  if (cmd == RTM_DELROUTE && !rib)
+  if (cmd == RTM_DELROUTE)
     goto skip;
 
-  if (rib)
-    ri->rtm_protocol = netlink_proto_from_route_type (rib->type);
+  if (!rib)
+    {
+      zlog_err("netlink_route_info_fill RTM_ADDROUTE called without rib info");
+      return 0;
+    }
+
+  ri->rtm_protocol = netlink_proto_from_route_type (rib->type);
 
   if ((rib->flags & ZEBRA_FLAG_BLACKHOLE) || (rib->flags & ZEBRA_FLAG_REJECT))
     discard = 1;