Re: [PATCH net 1/2] net: do not keep the dst cache when uncloning an skb dst and its metadata

[Antoine Tenart <atenart@kernel.org>]

Quoting Daniel Borkmann (2022-02-02 13:13:30)
> On 2/2/22 12:01 PM, Antoine Tenart wrote:
> > When uncloning an skb dst and its associated metadata a new dst+metadata
> > is allocated and the tunnel information from the old metadata is copied
> > over there.
> > 
> > The issue is the tunnel metadata has references to cached dst, which are
> > copied along the way. When a dst+metadata refcount drops to 0 the
> > metadata is freed including the cached dst entries. As they are also
> > referenced in the initial dst+metadata, this ends up in UaFs.
> > 
> > In practice the above did not happen because of another issue, the
> > dst+metadata was never freed because its refcount never dropped to 0
> > (this will be fixed in a subsequent patch).
> > 
> > Fix this by initializing the dst cache after copying the tunnel
> > information from the old metadata to also unshare the dst cache.
> > 
> > Fixes: d71785ffc7e7 ("net: add dst_cache to ovs vxlan lwtunnel")
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Reported-by: Vlad Buslov <vladbu@nvidia.com>
> > Tested-by: Vlad Buslov <vladbu@nvidia.com>
> > Signed-off-by: Antoine Tenart <atenart@kernel.org>
> > ---
> >   include/net/dst_metadata.h | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> > index 14efa0ded75d..c8f8b7b56bba 100644
> > --- a/include/net/dst_metadata.h
> > +++ b/include/net/dst_metadata.h
> > @@ -110,8 +110,8 @@ static inline struct metadata_dst *tun_rx_dst(int md_size)
> >   static inline struct metadata_dst *tun_dst_unclone(struct sk_buff *skb)
> >   {
> >       struct metadata_dst *md_dst = skb_metadata_dst(skb);
> > -     int md_size;
> >       struct metadata_dst *new_md;
> > +     int md_size, ret;
> >   
> >       if (!md_dst || md_dst->type != METADATA_IP_TUNNEL)
> >               return ERR_PTR(-EINVAL);
> > @@ -123,6 +123,17 @@ static inline struct metadata_dst *tun_dst_unclone(struct sk_buff *skb)
> >   
> >       memcpy(&new_md->u.tun_info, &md_dst->u.tun_info,
> >              sizeof(struct ip_tunnel_info) + md_size);
> > +#ifdef CONFIG_DST_CACHE
> > +     ret = dst_cache_init(&new_md->u.tun_info.dst_cache, GFP_ATOMIC);
> > +     if (ret) {
> > +             /* We can't call metadata_dst_free directly as the still shared
> > +              * dst cache would be released.
> > +              */
> > +             kfree(new_md);
> > +             return ERR_PTR(ret);
> > +     }
> > +#endif
> 
> Could you elaborate (e.g. also in commit message) how this interacts
> or whether it is needed for TUNNEL_NOCACHE users? (Among others,
> latter is used by BPF, for example.)

My understanding is that TUNNEL_NOCACHE is used to decide whether or not
to use a dst cache, that might or might not come from the tunnel info
attached to an skb. The dst cache being allocated in a tunnel info is
orthogonal to the use of TUNNEL_NOCACHE. While looking around I actually
found a code path explicitly setting both, in nft_tunnel_obj_init (that
might need to be investigated though but it is another topic).

It doesn't look like initializing the dst cache would break
TUNNEL_NOCACHE users as ip_tunnel_dst_cache_usable would return false
anyway. Having said that, we probably want to unshare the dst cache only
if there is one already, checking for
'md_dst->u.tun_info.dst_cache.cache != NULL' first.

Does that make sense?

Thanks!
Antoine