[SRU][Cosmic][Bionic][Xenial][PATCH 0/2] Fixes for LP1800639

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Cosmic][Bionic][Xenial][PATCH 0/2] Fixes for LP1800639

Frank Heimes

== SRU Justification ==

Fix socket buffer (skb) leaks for HiperTransport

Description: net/af_iucv: fix skb leaks for HiperTransport
Symptom: Memory leaks and/or double-freed network packets.
Problem: Inbound packets may have any combination of flag bits set in
their iucv header. Current code only handles certain
combinations, and ignores (ie. leaks) all packets with other flags.

On Transmit, current code is inconsistent about whether the error
paths need to free the skb. Depending on which error path is
taken, it may either get freed twice, or leak.
Solution: On receive, drop any skb with an unexpected combination of iucv
Header flags.
On transmit, be consistent in all error paths about free'ing the skb.

== Fix ==

222440996d6daf635bed6cb35041be22ede3e8a0 ("net/af_iucv: drop inbound packets with invalid flags")
b2f543949acd1ba64313fdad9e672ef47550d773 ("net/af_iucv: fix skb handling on HiperTransport xmit error")

== Patch ==

commit 222440996d6daf635bed6cb35041be22ede3e8a0
Author: Julian Wiedmann <[hidden email]>
Date:   Wed Sep 5 16:55:10 2018 +0200

    net/af_iucv: drop inbound packets with invalid flags
    
    Inbound packets may have any combination of flag bits set in their iucv
    header. If we don't know how to handle a specific combination, drop the
    skb instead of leaking it.
    
    To clarify what error is returned in this case, replace the hard-coded
    0 with the corresponding macro.
    
    Signed-off-by: Julian Wiedmann <[hidden email]>
    Signed-off-by: David S. Miller <[hidden email]>

==

commit b2f543949acd1ba64313fdad9e672ef47550d773
Author: Julian Wiedmann <[hidden email]>
Date:   Wed Sep 5 16:55:11 2018 +0200

    net/af_iucv: fix skb handling on HiperTransport xmit error
    
    When sending an skb, afiucv_hs_send() bails out on various error
    conditions. But currently the caller has no way of telling whether the
    skb was freed or not - resulting in potentially either
    a) leaked skbs from iucv_send_ctrl(), or
    b) double-free's from iucv_sock_sendmsg().
    
    As dev_queue_xmit() will always consume the skb (even on error), be
    consistent and also free the skb from all other error paths. This way
    callers no longer need to care about managing the skb.
    
    Signed-off-by: Julian Wiedmann <[hidden email]>
    Reviewed-by: Ursula Braun <[hidden email]>
    Signed-off-by: David S. Miller <[hidden email]>

== Regression Potential ==

Low, because:
- IUCV functionality is very special to s390x and is only supported in z/VM environments
  (z/VM hypervisor to guest or guest to guest communications)
- So everything is s390x specific.
- Patch is limited to this single file: /net/iucv/af_iucv.c
- A problem situation was identified by IBM
  that was then fixed, the fix tested and now needs to roled out as preventive fix.

== Test Case ==

Set IUCV communication on an Ubuntu s390x system that runs as z/VM guest:
Provoke an error situation.
This is btw. hard to do, because the 'Inter-User Communication Vehicle" (IUCV) is a virtual z/VM internal
network that does not use any real media.
To check for regressions one can use a shell over an ssh connection using an IUCV interface
or use an application that utilises AF_IUCV sockets (like ICC).

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Cosmic][Bionic][Xenial][PATCH 1/2] net/af_iucv: drop inbound packets with invalid flags

Frank Heimes
From: Julian Wiedmann <[hidden email]>


    net/af_iucv: drop inbound packets with invalid flags
    
    Inbound packets may have any combination of flag bits set in their iucv
    header. If we don't know how to handle a specific combination, drop the
    skb instead of leaking it.
    
    To clarify what error is returned in this case, replace the hard-coded
    0 with the corresponding macro.
    
    Signed-off-by: Julian Wiedmann <[hidden email]>
    Signed-off-by: David S. Miller <[hidden email]>

---

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index a21d8ed..01000c1 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2155,8 +2155,8 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
        struct sock *sk;
        struct iucv_sock *iucv;
        struct af_iucv_trans_hdr *trans_hdr;
+       int err = NET_RX_SUCCESS;
        char nullstring[8];
-       int err = 0;
 
        if (skb->len < (ETH_HLEN + sizeof(struct af_iucv_trans_hdr))) {
                WARN_ONCE(1, "AF_IUCV too short skb, len=%d, min=%d",
@@ -2254,7 +2254,7 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
                err = afiucv_hs_callback_rx(sk, skb);
                break;
        default:
-               ;
+               kfree_skb(skb);
        }
 
        return err;


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Cosmic][Bionic][Xenial][PATCH 2/2] net/af_iucv: fix skb handling on HiperTransport xmit error

Frank Heimes
In reply to this post by Frank Heimes
From: Julian Wiedmann <[hidden email]>


    net/af_iucv: fix skb handling on HiperTransport xmit error
    
    When sending an skb, afiucv_hs_send() bails out on various error
    conditions. But currently the caller has no way of telling whether the
    skb was freed or not - resulting in potentially either
    a) leaked skbs from iucv_send_ctrl(), or
    b) double-free's from iucv_sock_sendmsg().
    
    As dev_queue_xmit() will always consume the skb (even on error), be
    consistent and also free the skb from all other error paths. This way
    callers no longer need to care about managing the skb.
    
    Signed-off-by: Julian Wiedmann <[hidden email]>
    Reviewed-by: Ursula Braun <[hidden email]>
    Signed-off-by: David S. Miller <[hidden email]>

---

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 01000c1..e2f16a0 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -351,20 +351,28 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
                memcpy(&phs_hdr->iucv_hdr, imsg, sizeof(struct iucv_message));
 
        skb->dev = iucv->hs_dev;
-       if (!skb->dev)
-               return -ENODEV;
-       if (!(skb->dev->flags & IFF_UP) || !netif_carrier_ok(skb->dev))
-               return -ENETDOWN;
+       if (!skb->dev) {
+               err = -ENODEV;
+               goto err_free;
+       }
+       if (!(skb->dev->flags & IFF_UP) || !netif_carrier_ok(skb->dev)) {
+               err = -ENETDOWN;
+               goto err_free;
+       }
        if (skb->len > skb->dev->mtu) {
-               if (sock->sk_type == SOCK_SEQPACKET)
-                       return -EMSGSIZE;
-               else
-                       skb_trim(skb, skb->dev->mtu);
+               if (sock->sk_type == SOCK_SEQPACKET) {
+                       err = -EMSGSIZE;
+                       goto err_free;
+               }
+               skb_trim(skb, skb->dev->mtu);
        }
        skb->protocol = cpu_to_be16(ETH_P_AF_IUCV);
        nskb = skb_clone(skb, GFP_ATOMIC);
-       if (!nskb)
-               return -ENOMEM;
+       if (!nskb) {
+               err = -ENOMEM;
+               goto err_free;
+       }
+
        skb_queue_tail(&iucv->send_skb_q, nskb);
        err = dev_queue_xmit(skb);
        if (net_xmit_eval(err)) {
@@ -375,6 +383,10 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
                WARN_ON(atomic_read(&iucv->msg_recv) < 0);
        }
        return net_xmit_eval(err);
+
+err_free:
+       kfree_skb(skb);
+       return err;
 }
 
 static struct sock *__iucv_get_sock_by_name(char *nm)
@@ -1167,7 +1179,7 @@ static int iucv_sock_sendmsg(struct socket *sock, struct msghdr *msg,
                err = afiucv_hs_send(&txmsg, sk, skb, 0);
                if (err) {
                        atomic_dec(&iucv->msg_sent);
-                       goto fail;
+                       goto out;
                }
        } else { /* Classic VM IUCV transport */
                skb_queue_tail(&iucv->send_skb_q, skb);

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NAK: [SRU][Cosmic][Bionic][Xenial][PATCH 0/2] Fixes for LP1800639

Frank Heimes
In reply to this post by Frank Heimes
Did not perfectly fit to the Stable Patch Format.
Hence closing (NAK) this and will re-submit as v2 with corrections.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team