Hi Daniel,
ok, here is my version with separate sk_filter() call in af_iucv:
---
net/iucv/af_iucv.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 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);
+ if (sk_filter(sk, skb)) {
+ atomic_inc(&sk->sk_drops); /* skb rejected by filter */
+ kfree_skb(skb);
+ return;
+ }
+ if (__sock_queue_rcv_skb(sk, skb)) /* 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,13 @@ 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)) {
+ if (__sock_queue_rcv_skb(sk, rskb)) {
+ /* 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))
@@ -2116,12 +2121,17 @@ static int afiucv_hs_callback_rx(struct
skb_reset_transport_header(skb);
skb_reset_network_header(skb);
IUCV_SKB_CB(skb)->offset = 0;
+ if (sk_filter(sk, skb)) {
+ atomic_inc(&sk->sk_drops); /* skb rejected by filter */
+ kfree_skb(skb);
+ return NET_RX_SUCCESS;
+ }
+
spin_lock(&iucv->message_q.lock);
if (skb_queue_empty(&iucv->backlog_skb_q)) {
- if (sock_queue_rcv_skb(sk, skb)) {
+ 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);
Thanks, Ursula
On 07/18/2016 02:34 PM, Daniel Borkmann wrote:
Hi Ursula,
On 07/18/2016 02:27 PM, Ursula Braun wrote:
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:
Thanks for working on this. A comment below:
---
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;
I'd suggest to make this slightly different in the sense that it
would be better to split out sk_filter() to an appropriate spot in
the RX path and use for the other cases the __sock_queue_rcv_skb()
variant.
My worry is that return codes from sk_filter() could potentially
change or would be hard to track (e.g. could also origin from
security modules, etc called by sk_filter()), and only really
errors from __sock_queue_rcv_skb() are the ones you want to handle
by re-queuing here.
Thanks,
Daniel
+ }
+ 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
