[ovs-dev] [PATCH v2 1/2] tun_metadata: Fix coredump caused by use-after-free bug

Yifeng Sun pkusunyifeng at gmail.com
Wed Apr 8 15:16:06 UTC 2020


ovsrcu_set() is not necessary here because frozen_flow is on stack.

On Wed, Apr 8, 2020 at 6:48 AM William Tu <u9012063 at gmail.com> wrote:

> On Tue, Apr 07, 2020 at 02:57:03PM -0700, Yi-Hung Wei wrote:
> > On Tue, Apr 7, 2020 at 2:50 PM Yifeng Sun <pkusunyifeng at gmail.com>
> wrote:
> > >
> > > Tun_metadata can be referened by flow and frozen_state at the same
> > > time. When ovs-vswitchd handles TLV table mod message, the involved
> > > tun_metadata gets freed. The call trace to free tun_metadata is
> > > shown as below:
> > >
> > > ofproto_run
> > > - handle_openflow
> > >   - handle_single_part_openflow
> > >     - handle_tlv_table_mod
> > >       - tun_metadata_table_mod
> > >         - tun_metadata_postpone_free
> > >
> > > Unfortunately, this tun_metadata can be still used by some
> frozen_state,
> > > and later on when frozen_state tries to access its tun_metadata table,
> > > ovs-vswitchd crashes. The call trace to access tun_metadata from
> > > frozen_state is shown as below:
> > >
> > > udpif_upcall_handler
> > > - recv_upcalls
> > >   - process_upcall
> > >     - frozen_metadata_to_flow
> > >
> > > It is unsafe for frozen_state to reference tun_table because tun_table
> > > is protected by RCU while the lifecycle of frozen_state can span
> several
> > > RCU quiesce states. Current code violates OVS's RCU protection
> mechanism.
> > >
> > > This patch fixes it by simply stopping frozen_state from referencing
> > > tun_table. If frozen_state needs tun_table, we can find the latest
> valid
> > > tun_table through ofproto_get_tun_tab() efficiently.
> > >
> > > A previous commit seems fixing the samiliar issue:
> > > 254878c18874f6 (ofproto-dpif-xlate: Fix segmentation fault caused by
> tun_table)
> > >
> > > VMware-BZ: #2526222
> > > Signed-off-by: Yifeng Sun <pkusunyifeng at gmail.com>
> > > ---
> > > v1->v2: Drop the fix based on reference count. It doesn't fit well
> with RCU
> > > mechanism. Thanks William and YiHung for the offline discussion.
> > >
> > >  ofproto/ofproto-dpif-rid.h    | 7 +++++++
> > >  ofproto/ofproto-dpif-upcall.c | 2 ++
> > >  2 files changed, 9 insertions(+)
> > >
> > > diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
> > > index e5d02caf28a3..5235764a9885 100644
> > > --- a/ofproto/ofproto-dpif-rid.h
> > > +++ b/ofproto/ofproto-dpif-rid.h
> > > @@ -115,6 +115,13 @@ frozen_metadata_from_flow(struct frozen_metadata
> *md,
> > >  {
> > >      memset(md, 0, sizeof *md);
> > >      md->tunnel = flow->tunnel;
> > > +    /* It is unsafe for frozen_state to reference tun_table because
> > > +     * tun_table is protected by RCU while the lifecycle of
> frozen_state
> > > +     * can span several RCU quiesce states.
> > > +     *
> > > +     * The latest valid tun_table can be found by
> ofproto_get_tun_tab()
> > > +     * efficiently. */
> > > +    md->tunnel.metadata.tab = NULL;
>
> tun_table is RCU-protected, should we use ovsrcu_set?
>
> > >      md->metadata = flow->metadata;
> > >      memcpy(md->regs, flow->regs, sizeof md->regs);
> > >      md->in_port = flow->in_port.ofp_port;
> > > diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > > index 8dfa05b71df4..949cd4dbaf6f 100644
> > > --- a/ofproto/ofproto-dpif-upcall.c
> > > +++ b/ofproto/ofproto-dpif-upcall.c
> > > @@ -1535,6 +1535,8 @@ process_upcall(struct udpif *udpif, struct
> upcall *upcall,
> > >              }
> > >
> > >              frozen_metadata_to_flow(&state->metadata, &frozen_flow);
> > > +            frozen_flow.tunnel.metadata.tab = ofproto_get_tun_tab(
> > > +                &upcall->ofproto->up);
> >
> >
> > Thanks for the fix.  I wonder if it makes sense to move
> > ofproto_get_tun_tab() into frozen_metadata_to_flow()?  Therefore, we
> > do not need to call ofproto_get_tun_tab() to reset the tun_table for
> > other frozen state use case.
> >
> > Thanks,
> >
> > -Yi-Hung
> >
> >
> > >              flow_get_metadata(&frozen_flow,
> &am->pin.up.base.flow_metadata);
> > >
> > >              ofproto_dpif_send_async_msg(upcall->ofproto, am);
> > > --
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list