Hi Daniel,
thanks for reporting this. I agree, our af_iucv code is just prepared to
handle sock_queue_rcv_skb() return codes -ENOMEM and -ENOBUFS in a
meaningful way , but it does not handle return code -EPERM from the
sk_filter() call in sock_queue_rcv_skb().
Here is my idea how to fix this:
---
net/iucv/af_iucv.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1315,8 +1315,13 @@ static void iucv_process_message(struct
}
IUCV_SKB_CB(skb)->offset = 0;
- if (sock_queue_rcv_skb(sk, skb))
- skb_queue_head(&iucv_sk(sk)->backlog_skb_q, skb);
+ rc = sock_queue_rcv_skb(sk, skb);
+ if (rc == -EPERM) {
+ kfree_skb(skb); /* skb rejected by filter */
+ return;
+ }
+ if (rc) /* handle rcv queue full */
+ skb_queue_tail(&iucv_sk(sk)->backlog_skb_q, skb);
}
/* iucv_process_message_q() - Process outstanding IUCV messages
@@ -1430,13 +1435,16 @@ static int iucv_sock_recvmsg(struct sock
rskb = skb_dequeue(&iucv->backlog_skb_q);
while (rskb) {
IUCV_SKB_CB(rskb)->offset = 0;
- if (sock_queue_rcv_skb(sk, rskb)) {
+ err = sock_queue_rcv_skb(sk, rskb);
+ if (err == -EPERM) { /* skb rejected by filter */
+ kfree_skb(skb);
+ } else if (err) {
+ /* handle rcv queue full */
skb_queue_head(&iucv->backlog_skb_q,
rskb);
break;
- } else {
- rskb = skb_dequeue(&iucv->backlog_skb_q);
}
+ rskb = skb_dequeue(&iucv->backlog_skb_q);
}
if (skb_queue_empty(&iucv->backlog_skb_q)) {
if (!list_empty(&iucv->message_q.list))
@@ -2118,12 +2126,19 @@ static int afiucv_hs_callback_rx(struct
IUCV_SKB_CB(skb)->offset = 0;
spin_lock(&iucv->message_q.lock);
if (skb_queue_empty(&iucv->backlog_skb_q)) {
- if (sock_queue_rcv_skb(sk, skb)) {
+ int err = sock_queue_rcv_skb(sk, skb);
+
+ if (err == -EPERM) {
+ kfree_skb(skb); /* skb rejected by filter */
+ goto out;
+ }
+ if (err) {
/* handle rcv queue full */
skb_queue_tail(&iucv->backlog_skb_q, skb);
}
} else
skb_queue_tail(&iucv_sk(sk)->backlog_skb_q, skb);
+out:
spin_unlock(&iucv->message_q.lock);
return NET_RX_SUCCESS;
}
Kind regards, Ursula
On 07/15/2016 11:46 PM, Daniel Borkmann wrote:
Hi folks,
during an audit of sk_filter() users, I stumbled upon af_iucv, where
I suppose some weird things are going on when socket filters are
attached to an af_iucv socket (f.e. setsockopt() with SO_ATTACH_FILTER).
You have three invocation of sock_queue_rcv_skb():
*** iucv_process_message():
[...]
IUCV_SKB_CB(skb)->offset = 0;
if (sock_queue_rcv_skb(sk, skb))
skb_queue_head(&iucv_sk(sk)->backlog_skb_q, skb);
[...]
*** iucv_sock_recvmsg():
[...]
spin_lock_bh(&iucv->message_q.lock);
rskb = skb_dequeue(&iucv->backlog_skb_q);
while (rskb) {
IUCV_SKB_CB(rskb)->offset = 0;
if (sock_queue_rcv_skb(sk, rskb)) {
skb_queue_head(&iucv->backlog_skb_q, rskb);
break;
} else {
rskb = skb_dequeue(&iucv->backlog_skb_q);
}
}
[...]
spin_unlock_bh(&iucv->message_q.lock);
[...]
**** afiucv_hs_callback_rx():
[...]
spin_lock(&iucv->message_q.lock);
if (skb_queue_empty(&iucv->backlog_skb_q)) {
if (sock_queue_rcv_skb(sk, skb)) {
/* handle rcv queue full */
skb_queue_tail(&iucv->backlog_skb_q, skb);
}
} else
skb_queue_tail(&iucv_sk(sk)->backlog_skb_q, skb);
spin_unlock(&iucv->message_q.lock);
return NET_RX_SUCCESS;
[...]
As you probably know, sock_queue_rcv_skb() calls into sk_filter(),
then into __sock_queue_rcv_skb() for actual queuing. Now, when for
example a cBPF filter is attached where some path of it ends up with ...
{ 0x06, 0, 0, 0x00000000 }, // ret K (K := 0)
... is attached, for example, then due to return code 0, it means
skb will be tossed, and as a result sk_filter() will return -EPERM.
In all above cases, this means sock_queue_rcv_skb() will return a
non-0 code, and thus you either re-queue the skb to the head or tail
of the backlog queue. So, in case of head, no matter what other skbs
come in, the previous 'to-be-dropped' skb will be dequeued and put
back again into the backlog queue's head. tail case, well, it will
probably let the skb linger in that queue until socket destruction.
What you probably meant for all three is to just call
__sock_queue_rcv_skb()
and place a sk_filter() call to some safe to use location or so.
Please have a look / fix at this, if I'm not mistaken.
Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
