[ovs-dev] [PATCH] ofproto-dpif-upcall: Fix using uninitialized upcall hash.

William Tu u9012063 at gmail.com
Mon Jan 6 20:27:43 UTC 2020


On Sat, Jan 04, 2020 at 11:16:27AM +0800, Tonghao Zhang wrote:
> On Sat, Jan 4, 2020 at 8:16 AM Ilya Maximets <i.maximets at ovn.org> wrote:
> >
> > upcalls are allocated on stack and 'hash' field must be initialized
> > regardless of attribute existence because it will be used later.
> >
> >  Conditional jump or move depends on uninitialised value(s)
> >     at 0xFA74A7: dpif_netlink_encode_execute (dpif-netlink.c:1828)
> >     by 0xFA6DE8: dpif_netlink_operate__ (dpif-netlink.c:1906)
> >     by 0xFA612F: dpif_netlink_operate_chunks (dpif-netlink.c:2219)
> >     by 0xFA0E36: dpif_netlink_operate (dpif-netlink.c:2275)
> >     by 0xE5AFAC: dpif_operate (dpif.c:1376)
> >     by 0xDF3922: handle_upcalls (ofproto-dpif-upcall.c:1615)
> >     by 0xDF269B: recv_upcalls (ofproto-dpif-upcall.c:857)
> >     by 0xDF1C49: udpif_upcall_handler (ofproto-dpif-upcall.c:759)
> >     by 0xF3A3FE: ovsthread_wrapper (ovs-thread.c:383)
> >     by 0x565F6DA: start_thread (pthread_create.c:463)
> >     by 0x615988E: clone (clone.S:95)
> >   Uninitialised value was created by a stack allocation
> >     at 0xDF2258: recv_upcalls (ofproto-dpif-upcall.c:773)
> >
> > Fixes: 0442bfb11d6c ("ofproto-dpif-upcall: Echo HASH attribute back to datapath.")
> > Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
> > ---
> >  ofproto/ofproto-dpif-upcall.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> > index aac0554af..409286ab1 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -786,6 +786,7 @@ recv_upcalls(struct handler *handler)
> >          struct upcall *upcall = &upcalls[n_upcalls];
> >          struct flow *flow = &flows[n_upcalls];
> >          unsigned int mru = 0;
> > +        uint64_t hash = 0;
> >          int error;
> >
> >          ofpbuf_use_stub(recv_buf, recv_stubs[n_upcalls],
> > @@ -806,7 +807,7 @@ recv_upcalls(struct handler *handler)
> >          }
> >
> >          if (dupcall->hash) {
> > -            upcall->hash = nl_attr_get_u64(dupcall->hash);
> > +            hash = nl_attr_get_u64(dupcall->hash);
> >          }
> >
> >          error = upcall_receive(upcall, udpif->backer, &dupcall->packet,
> > @@ -830,6 +831,7 @@ recv_upcalls(struct handler *handler)
> >          upcall->key = dupcall->key;
> >          upcall->key_len = dupcall->key_len;
> >          upcall->ufid = &dupcall->ufid;
> > +        upcall->hash = hash;
> >
> >          upcall->out_tun_key = dupcall->out_tun_key;
> >          upcall->actions = dupcall->actions;
> > --
> > 2.17.1
> >
> Acked-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>

Looks good to me.
Acked-by: William Tu <u9012063 at gmail.com>

I assume you're running valgrind and saw this.
I'm curious which test cases triggers this issue?
Thanks
William


More information about the dev mailing list