| Message ID | 20220207171319.157775-2-atenart@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cfc56f85e72f5b9c5c5be26dc2b16518d36a7868 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fix issues when uncloning an skb dst+metadata | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 75 this patch: 75 |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/build_clang | success | Errors and warnings before: 25 this patch: 25 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 94 this patch: 94 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 19 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, 2022-02-07 at 18:13 +0100, 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, 13 insertions(+) > > diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h > index 14efa0ded75d..b997e0c1e362 100644 > --- a/include/net/dst_metadata.h > +++ b/include/net/dst_metadata.h > @@ -123,6 +123,19 @@ 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 > + /* Unclone the dst cache if there is one */ > + if (new_md->u.tun_info.dst_cache.cache) { > + int ret; > + > + ret = dst_cache_init(&new_md->u.tun_info.dst_cache, GFP_ATOMIC); > + if (ret) { > + metadata_dst_free(new_md); > + return ERR_PTR(ret); > + } > + } > +#endif > + > skb_dst_drop(skb); > dst_hold(&new_md->dst); > skb_dst_set(skb, &new_md->dst); LGTM, thanks! Acked-by: Paolo Abeni <pabeni@redhat.com>
diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h index 14efa0ded75d..b997e0c1e362 100644 --- a/include/net/dst_metadata.h +++ b/include/net/dst_metadata.h @@ -123,6 +123,19 @@ 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 + /* Unclone the dst cache if there is one */ + if (new_md->u.tun_info.dst_cache.cache) { + int ret; + + ret = dst_cache_init(&new_md->u.tun_info.dst_cache, GFP_ATOMIC); + if (ret) { + metadata_dst_free(new_md); + return ERR_PTR(ret); + } + } +#endif + skb_dst_drop(skb); dst_hold(&new_md->dst); skb_dst_set(skb, &new_md->dst);
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, 13 insertions(+)