netdev - Re: [PATCH net v2] tun: fix ubuf refcount incorrectly on error path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 12 Dec 2020 19:17:39 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     wangyunjian <wangyunjian@...wei.com>
Cc:     "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        Network Development <netdev@...r.kernel.org>,
        "Lilijun (Jerry)" <jerry.lilijun@...wei.com>,
        chenchanghu <chenchanghu@...wei.com>,
        xudingke <xudingke@...wei.com>,
        "huangbin (J)" <brian.huangbin@...wei.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net v2] tun: fix ubuf refcount incorrectly on error path

> > > afterwards, the error handling in vhost handle_tx() will try to
> > > decrease the same refcount again. This is wrong and fix this by delay
> > > copying ubuf_info until we're sure there's no errors.
> >
> > I think the right approach is to address this in the error paths, rather than
> > complicate the normal datapath.
> >
> > Is it sufficient to suppress the call to vhost_net_ubuf_put in the handle_tx
> > sendmsg error path, given that vhost_zerocopy_callback will be called on
> > kfree_skb?
>
> We can not call kfree_skb() until the skb was created.
>
> >
> > Or alternatively clear the destructor in drop:
>
> The uarg->callback() is called immediately after we decide do datacopy
> even if caller want to do zerocopy. If another error occurs later, the vhost
> handle_tx() will try to decrease it again.

Oh right, I missed the else branch in this path:

        /* copy skb_ubuf_info for callback when skb has no error */
        if (zerocopy) {
                skb_shinfo(skb)->destructor_arg = msg_control;
                skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
                skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
        } else if (msg_control) {
                struct ubuf_info *uarg = msg_control;
                uarg->callback(uarg, false);
        }

So if handle_tx_zerocopy calls tun_sendmsg with ubuf_info (and thus a
reference to release), there are these five options:

1. tun_sendmsg succeeds, ubuf_info is associated with skb.
     reference released from kfree_skb calling vhost_zerocopy_callback later

2. tun_sendmsg succeeds, ubuf_info is released immediately, as skb is
not zerocopy.

3. tun_sendmsg fails before creating skb, handle_tx_zerocopy correctly
cleans up on receiving error from tun_sendmsg.

4. tun_sendmsg fails after creating skb, but with copying: decremented
at branch shown above + again in handle_tx_zerocopy

5. tun_sendmsg fails after creating skb, with zerocopy: decremented at
kfree_skb in drop: + again in handle_tx_zerocopy

Since handle_tx_zerocopy has no idea whether on error 3, 4 or 5
occurred, either all decrement-on-error cases must be handled by
handle_tx_zerocopy or none.

Your patch chooses the latter. Makes sense.

But can this still go wrong if the xdp path is taken, but no program
exists or the program returns XDP_PASS. And then the packet hits an
error path, such as ! IFF_UP?