Message ID | 1541515594-22183-2-git-send-email-gavin.guo@canonical.com |
---|---|
State | New |
Headers | show |
Series | [SRU,Xenial,Bionic,Cosmic] xfrm: Fix NULL pointer dereference when skb_dst_force clears the dst_entry. | expand |
On 11/06/18 15:46, Gavin Guo wrote: > From: Steffen Klassert <steffen.klassert@secunet.com> > > BugLink: https://bugs.launchpad.net/bugs/1801878 > > Since commit 222d7dbd258d ("net: prevent dst uses after free") > skb_dst_force() might clear the dst_entry attached to the skb. > The xfrm code don't expect this to happen, so we crash with > a NULL pointer dereference in this case. Fix it by checking > skb_dst(skb) for NULL after skb_dst_force() and drop the packet > in cast the dst_entry was cleared. > > Fixes: 222d7dbd258d ("net: prevent dst uses after free") > Reported-by: Tobias Hommel <netdev-list@genoetigt.de> > Reported-by: Kristian Evensen <kristian.evensen@gmail.com> > Reported-by: Wolfgang Walter <linux@stwm.de> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > (cherry picked from commit 9e1437937807b0122e8da1ca8765be2adca9aee6) > Signed-off-by: Gavin Guo <gavin.guo@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > net/xfrm/xfrm_output.c | 4 ++++ > net/xfrm/xfrm_policy.c | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c > index 35610cc881a9..c47660fba498 100644 > --- a/net/xfrm/xfrm_output.c > +++ b/net/xfrm/xfrm_output.c > @@ -101,6 +101,10 @@ static int xfrm_output_one(struct sk_buff *skb, int err) > spin_unlock_bh(&x->lock); > > skb_dst_force(skb); > + if (!skb_dst(skb)) { > + XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR); > + goto error_nolock; > + } > > if (xfrm_offload(skb)) { > x->type_offload->encap(x, skb); > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 7bcd6f1b86a0..1690efebba8a 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -2544,6 +2544,10 @@ int __xfrm_route_forward(struct sk_buff *skb, unsigned short family) > } > > skb_dst_force(skb); > + if (!skb_dst(skb)) { > + XFRM_INC_STATS(net, LINUX_MIB_XFRMFWDHDRERROR); > + return 0; > + } > > dst = xfrm_lookup(net, skb_dst(skb), &fl, NULL, XFRM_LOOKUP_QUEUE); > if (IS_ERR(dst)) {
On 06.11.18 15:46, Gavin Guo wrote: > From: Steffen Klassert <steffen.klassert@secunet.com> > > BugLink: https://bugs.launchpad.net/bugs/1801878 > > Since commit 222d7dbd258d ("net: prevent dst uses after free") > skb_dst_force() might clear the dst_entry attached to the skb. > The xfrm code don't expect this to happen, so we crash with > a NULL pointer dereference in this case. Fix it by checking > skb_dst(skb) for NULL after skb_dst_force() and drop the packet > in cast the dst_entry was cleared. > > Fixes: 222d7dbd258d ("net: prevent dst uses after free") > Reported-by: Tobias Hommel <netdev-list@genoetigt.de> > Reported-by: Kristian Evensen <kristian.evensen@gmail.com> > Reported-by: Wolfgang Walter <linux@stwm.de> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > (cherry picked from commit 9e1437937807b0122e8da1ca8765be2adca9aee6) > Signed-off-by: Gavin Guo <gavin.guo@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > net/xfrm/xfrm_output.c | 4 ++++ > net/xfrm/xfrm_policy.c | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c > index 35610cc881a9..c47660fba498 100644 > --- a/net/xfrm/xfrm_output.c > +++ b/net/xfrm/xfrm_output.c > @@ -101,6 +101,10 @@ static int xfrm_output_one(struct sk_buff *skb, int err) > spin_unlock_bh(&x->lock); > > skb_dst_force(skb); > + if (!skb_dst(skb)) { > + XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR); > + goto error_nolock; > + } > > if (xfrm_offload(skb)) { > x->type_offload->encap(x, skb); > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 7bcd6f1b86a0..1690efebba8a 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -2544,6 +2544,10 @@ int __xfrm_route_forward(struct sk_buff *skb, unsigned short family) > } > > skb_dst_force(skb); > + if (!skb_dst(skb)) { > + XFRM_INC_STATS(net, LINUX_MIB_XFRMFWDHDRERROR); > + return 0; > + } > > dst = xfrm_lookup(net, skb_dst(skb), &fl, NULL, XFRM_LOOKUP_QUEUE); > if (IS_ERR(dst)) { >
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c index 35610cc881a9..c47660fba498 100644 --- a/net/xfrm/xfrm_output.c +++ b/net/xfrm/xfrm_output.c @@ -101,6 +101,10 @@ static int xfrm_output_one(struct sk_buff *skb, int err) spin_unlock_bh(&x->lock); skb_dst_force(skb); + if (!skb_dst(skb)) { + XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR); + goto error_nolock; + } if (xfrm_offload(skb)) { x->type_offload->encap(x, skb); diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 7bcd6f1b86a0..1690efebba8a 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -2544,6 +2544,10 @@ int __xfrm_route_forward(struct sk_buff *skb, unsigned short family) } skb_dst_force(skb); + if (!skb_dst(skb)) { + XFRM_INC_STATS(net, LINUX_MIB_XFRMFWDHDRERROR); + return 0; + } dst = xfrm_lookup(net, skb_dst(skb), &fl, NULL, XFRM_LOOKUP_QUEUE); if (IS_ERR(dst)) {