[ovs-dev] [PATCH] ofproto-dpif-xlate: fix crash when using multicast snooping

[Aaron Conole]

Greetings Thadeu,

Thadeu Lima de Souza Cascardo <cascardo at redhat.com> writes:

> The revalidator thread may set may_learn and call xlate_actions with no packet
> data. If the revalidated flow is IGMPv3 or MLD, vswitchd will crash when trying
> to access the NULL packet.
>
> Only process IGMP and MLD flows when there is a packet. This is a similar
> behavior than what we have for other special packets.

Just a question on the approach, would it make sense to check for the
bundle being null as well? I don't know if the case where packet == NULL
but bundle != NULL could ever exist so I could be off my rocker.

Alternately, maybe it's better to just patch the mcast functions to bail
early (maybe with a ratelimited warning) in this case, so that if bundle
!= NULL but packet == NULL, some of the mcast group LRU cache can be
updated? Don't know if that makes sense, either (maybe there's a
security reason not to do that?).

Thanks,
Aaron

> Not-Signed-off-yet: Thadeu Lima de Souza Cascardo <cascardo at redhat.com>
> Reported-by: Yi Ba <yby.developer at yahoo.com>
> Reported-at: http://openvswitch.org/pipermail/discuss/2016-January/020023.html
> Fixes: 06994f879c9d ("mcast-snooping: Add Multicast Listener Discovery support")
> ---
>
> Hi, Yi Ba.
>
> Can you test this patch for your mcast_snooping bug? Remember to enable
> OVS_ENABLE_SG_FIREWALL_MULTICAST again. In order to verify it's working, you can
> run ovs-vsctl get bridge br-ex mcast_snooping_enable.
>
> Thanks.
> Cascardo.
>
> ---
>  AUTHORS                      | 1 +
>  ofproto/ofproto-dpif-xlate.c | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 936394d..366b72f 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -412,6 +412,7 @@ Vishal Swarankar        vishal.swarnkar at gmail.com
>  Vjekoslav Brajkovic     balkan at cs.washington.edu
>  Voravit T.              voravit at kth.se
>  Yeming Zhao             zhaoyeming at gmail.com
> +Yi Ba                   yby.developer at yahoo.com
>  Ying Chen               yingchen at vmware.com
>  Yongqiang Liu           liuyq7809 at gmail.com
>  ZHANG Zhiming           zhangzhiming at yunshan.net.cn
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a6ea067..7195d45 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2409,7 +2409,7 @@ xlate_normal(struct xlate_ctx *ctx)
>          if (is_igmp(flow)) {
>              if (mcast_snooping_is_membership(flow->tp_src) ||
>                  mcast_snooping_is_query(flow->tp_src)) {
> -                if (ctx->xin->may_learn) {
> +                if (ctx->xin->may_learn && ctx->xin->packet) {
>                      update_mcast_snooping_table(ctx->xbridge, flow, vlan,
>                                                  in_xbundle, ctx->xin->packet);
>                  }
> @@ -2441,7 +2441,7 @@ xlate_normal(struct xlate_ctx *ctx)
>              return;
>          } else if (is_mld(flow)) {
>              ctx->xout->slow |= SLOW_ACTION;
> -            if (ctx->xin->may_learn) {
> +            if (ctx->xin->may_learn && ctx->xin->packet) {
>                  update_mcast_snooping_table(ctx->xbridge, flow, vlan,
>                                              in_xbundle, ctx->xin->packet);
>              }