Re: [PATCH] af_netlink: give correct bounds to dump skb for NLMSG_DONE

[Jason A. Donenfeld]

Erf, your patch doesn't handle what happens if len comes back
negative, but I'll fix it up and send a v2 using this approach.

I think I really prefer v1 though.

Jason

Re: [PATCH] af_netlink: give correct bounds to dump skb for NLMSG_DONE

[Jason A. Donenfeld]

Hi Johannes,

Yes indeed. It sacrifices 24 bytes for making things much less
complex. However, if you prefer increasing the complexity of the state
machine a bit instead, I suppose we could roll with this approach
instead...

Jason

Re: [PATCH] af_netlink: give correct bounds to dump skb for NLMSG_DONE

[Johannes Berg]

On Tue, 2017-11-07 at 20:29 +0900, Jason A. Donenfeld wrote:
> 
> This patch thus reserves and restores the required length for
> NLMSG_DONE during the call to the dump function.
> 

That basically removes that space though, even when the dump isn't
complete... wouldn't it be better to do something like this?

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f34750691c5c..fccf83598dab 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2183,9 +2183,15 @@ static int netlink_dump(struct sock *sk)
skb_reserve(skb, skb_tailroom(skb) - alloc_size);
netlink_skb_set_owner_r(skb, sk);
 
-   len = cb->dump(skb, cb);
+   /* if the dump is already done, just complete */
+   if (nlk->dump_done)
+   len = 0;
+   else
+   len = cb->dump(skb, cb);
+
+   nlk->dump_done = len == 0;
 
-   if (len > 0) {
+   if (len > 0 || skb_tailroom(skb) < nlmsg_total_size(sizeof(len))) {
mutex_unlock(nlk->cb_mutex);
 
if (sk_filter(sk, skb))
@@ -2196,7 +2202,7 @@ static int netlink_dump(struct sock *sk)
}
 
nlh = nlmsg_put_answer(skb, cb, NLMSG_DONE, sizeof(len), NLM_F_MULTI);
-   if (!nlh)
+   if (WARN_ON(!nlh))
goto errout_skb;
 
nl_dump_check_consistent(cb, nlh);
@@ -2273,6 +2279,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff 
*skb,
}
 
nlk->cb_running = true;
+   nlk->dump_done = false;
 
mutex_unlock(nlk->cb_mutex);
 
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 3490f2430532..91a3652d384f 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -33,6 +33,7 @@ struct netlink_sock {
wait_queue_head_t   wait;
boolbound;
boolcb_running;
+   booldump_done;
struct netlink_callback cb;
struct mutex*cb_mutex;
struct mutexcb_def_mutex;


https://p.sipsolutions.net/90574c3c0116d68a.txt

(untested)

johannes

Re: [PATCH] af_netlink: give correct bounds to dump skb for NLMSG_DONE

[Jason A. Donenfeld]

By the way, in case you're curious, here's the {up,down,cross}stream
WireGuard commit that works around it via its compat layer (a rat's
nest of hideous backports for all the weird kernels people want
WireGuard to run on, which I cannot wait to remove):

https://git.zx2c4.com/WireGuard/commit/?id=f689ea7acc23dc8e0968699d964ee382b04fbbe4

Particularly relavent here is the last chunk of that, which is part of
the automated test suite, which reproduces the issue by finding the
tightest possible packing.