[PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[Pablo Neira Ayuso]

Currently, it is not possible to use neither NLM_F_EXCL nor
NLM_F_REPLACE from genetlink. This is due to this checking in
genl_family_rcv_msg:

	if (nlh->nlmsg_flags & NLM_F_DUMP)

NLM_F_DUMP is NLM_F_MATCH|NLM_F_ROOT. Thus, if NLM_F_EXCL or
NLM_F_REPLACE flag is set, genetlink believes that you're
requesting a dump and it calls the .dumpit callback.

The solution that I propose is to refine this checking to
make it stricter:

	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP)

And given the combination NLM_F_REPLACE and NLM_F_EXCL does
not make sense to me, it removes the ambiguity.

There was a patch that tried to fix this some time ago (0ab03c2
netlink: test for all flags of the NLM_F_DUMP composite) but it
tried to resolve this ambiguity in *all* existing netlink subsystems,
not only genetlink. That patch was reverted since it broke iproute2,
which is using NLM_F_ROOT to request the dump of the routing cache.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
There is still one possibility of breaking user-space: if
the application only sets NLM_F_MATCH or NLM_F_ROOT to request
a dump, the dump operation will not work anymore.

To address this, I have elaborated a list of all existing
in-tree subsystems that provide genetlink interfaces that
could be affected by git grepping for the "\.dumpit" keyword.
Then, I have searched for the user-space code of those
genetlink interfaces, to make sure they are using NLM_F_DUMP,
this is the result:

* nl80211: the iw utility uses NLM_F_DUMP.
* openvswitch: version 1.10.0, lib/netlink-socket.c uses
  NLM_F_DUMP.
* nfc: I could just find a nfc-example.git tree:
  http://code.openbossa.org/?p=nfc/nfc-example.git;a=summary
  which looks good.
* netlabel: netlabel_tools-0.20 looks good.
* IPVS: ipvsadm from Simon Horman's git tree looks good.
* l2tp: iproute2 code looks good as well.
* drdb: drbd-8.4 looks fine, drbd-8.3 does not seem to use
  the genetlink interface.

So it seems recent code always stick to NLM_F_DUMP, which is
good.

 net/netlink/genetlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 2fd6dbe..145d145 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -571,7 +571,7 @@ static int genl_family_rcv_msg(struct genl_family *family,
 	    !capable(CAP_NET_ADMIN))
 		return -EPERM;

-	if (nlh->nlmsg_flags & NLM_F_DUMP) {
+	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = ops->dumpit,
 			.done = ops->done,
--
1.7.10.4

Re: [PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 29 Jul 2013 12:30:04 +0200

> Currently, it is not possible to use neither NLM_F_EXCL nor
> NLM_F_REPLACE from genetlink. This is due to this checking in
> genl_family_rcv_msg:
> 
> 	if (nlh->nlmsg_flags & NLM_F_DUMP)
> 
> NLM_F_DUMP is NLM_F_MATCH|NLM_F_ROOT. Thus, if NLM_F_EXCL or
> NLM_F_REPLACE flag is set, genetlink believes that you're
> requesting a dump and it calls the .dumpit callback.
> 
> The solution that I propose is to refine this checking to
> make it stricter:
> 
> 	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP)
> 
> And given the combination NLM_F_REPLACE and NLM_F_EXCL does
> not make sense to me, it removes the ambiguity.
> 
> There was a patch that tried to fix this some time ago (0ab03c2
> netlink: test for all flags of the NLM_F_DUMP composite) but it
> tried to resolve this ambiguity in *all* existing netlink subsystems,
> not only genetlink. That patch was reverted since it broke iproute2,
> which is using NLM_F_ROOT to request the dump of the routing cache.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Yes, I remember that old attempt to fix this.

Ok, let's see what happens when we limit the scope of this change
to just genetlink users.

I honestly can't believe that NLM_F_EXCL and NLM_F_REPLACE are
completely unusable in normal rtnetlink interfaces.

Re: [PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[Pablo Neira Ayuso]

Hi David!

On Tue, Jul 30, 2013 at 04:44:23PM -0700, David Miller wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Mon, 29 Jul 2013 12:30:04 +0200
> 
> > Currently, it is not possible to use neither NLM_F_EXCL nor
> > NLM_F_REPLACE from genetlink. This is due to this checking in
> > genl_family_rcv_msg:
> > 
> > 	if (nlh->nlmsg_flags & NLM_F_DUMP)
> > 
> > NLM_F_DUMP is NLM_F_MATCH|NLM_F_ROOT. Thus, if NLM_F_EXCL or
> > NLM_F_REPLACE flag is set, genetlink believes that you're
> > requesting a dump and it calls the .dumpit callback.
> > 
> > The solution that I propose is to refine this checking to
> > make it stricter:
> > 
> > 	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP)
> > 
> > And given the combination NLM_F_REPLACE and NLM_F_EXCL does
> > not make sense to me, it removes the ambiguity.
> > 
> > There was a patch that tried to fix this some time ago (0ab03c2
> > netlink: test for all flags of the NLM_F_DUMP composite) but it
> > tried to resolve this ambiguity in *all* existing netlink subsystems,
> > not only genetlink. That patch was reverted since it broke iproute2,
> > which is using NLM_F_ROOT to request the dump of the routing cache.
> > 
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> Yes, I remember that old attempt to fix this.
> 
> Ok, let's see what happens when we limit the scope of this change
> to just genetlink users.
>
> I honestly can't believe that NLM_F_EXCL and NLM_F_REPLACE are
> completely unusable in normal rtnetlink interfaces.

I guess you mean 'genetlink' instead of 'rtnetlink'.

This repo provides one genetlink example kernel module and userspace
utility using libmnl:

git clone git://1984.lsi.us.es/netlink-example

make
make userspace
insmod genlexample.ko

This registers 'nlex', an example genetlink family that allows you to
update the value of a variable in kernel-space.

To use it, we have to resolve the genetlink family-id first (sorry,
these examples are a bit rudimentary):

./genl-resolve nlex
family-id: 24
multicast -> id
"example" -> 7

OK, so it's family-id 24, let's update that variable in kernel-space
without NLM_F_EXCL:

./genl-change 24        # uses time(NULL) to update that variable
done

./genl-get 24
myvar=1375268456
done

Now if we try to make it with NLM_F_EXCL:

./genl-change 24 excl
mnl_cb_run: Operation not supported

My examples have no .dumpit callback. thus, genetlink was
misiterpreting this and it returned EOPNOTSUPP.

So nobody using genetlink could use these two flags so far.

Re: [PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Wed, 31 Jul 2013 13:12:15 +0200

> Hi David!
> 
> On Tue, Jul 30, 2013 at 04:44:23PM -0700, David Miller wrote:
>> From: Pablo Neira Ayuso <pablo@netfilter.org>
>> Date: Mon, 29 Jul 2013 12:30:04 +0200
>> 
>> > Currently, it is not possible to use neither NLM_F_EXCL nor
>> > NLM_F_REPLACE from genetlink. This is due to this checking in
>> > genl_family_rcv_msg:
>> > 
>> > 	if (nlh->nlmsg_flags & NLM_F_DUMP)
>> > 
>> > NLM_F_DUMP is NLM_F_MATCH|NLM_F_ROOT. Thus, if NLM_F_EXCL or
>> > NLM_F_REPLACE flag is set, genetlink believes that you're
>> > requesting a dump and it calls the .dumpit callback.
>> > 
>> > The solution that I propose is to refine this checking to
>> > make it stricter:
>> > 
>> > 	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP)
>> > 
>> > And given the combination NLM_F_REPLACE and NLM_F_EXCL does
>> > not make sense to me, it removes the ambiguity.
>> > 
>> > There was a patch that tried to fix this some time ago (0ab03c2
>> > netlink: test for all flags of the NLM_F_DUMP composite) but it
>> > tried to resolve this ambiguity in *all* existing netlink subsystems,
>> > not only genetlink. That patch was reverted since it broke iproute2,
>> > which is using NLM_F_ROOT to request the dump of the routing cache.
>> > 
>> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> 
>> Yes, I remember that old attempt to fix this.
>> 
>> Ok, let's see what happens when we limit the scope of this change
>> to just genetlink users.
>>
>> I honestly can't believe that NLM_F_EXCL and NLM_F_REPLACE are
>> completely unusable in normal rtnetlink interfaces.
> 
> I guess you mean 'genetlink' instead of 'rtnetlink'.

I really meant 'rtnetlink' and netlink in general.  As you stated, we
tried to make the same check for all netlink users, and it had to
be reverted because iproute2 uses NLM_F_ROOT to get a dump.

Therefore I don't see how NLM_F_REPLACE and NLM_F_EXCL can be used
at all, in those places, because the check is still "& NLM_F_DUMP"

Re: [PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[Pablo Neira Ayuso]

On Wed, Jul 31, 2013 at 05:03:48PM -0700, David Miller wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Wed, 31 Jul 2013 13:12:15 +0200
> 
> > Hi David!
> > 
> > On Tue, Jul 30, 2013 at 04:44:23PM -0700, David Miller wrote:
> >> From: Pablo Neira Ayuso <pablo@netfilter.org>
> >> Date: Mon, 29 Jul 2013 12:30:04 +0200
> >> 
> >> > Currently, it is not possible to use neither NLM_F_EXCL nor
> >> > NLM_F_REPLACE from genetlink. This is due to this checking in
> >> > genl_family_rcv_msg:
> >> > 
> >> > 	if (nlh->nlmsg_flags & NLM_F_DUMP)
> >> > 
> >> > NLM_F_DUMP is NLM_F_MATCH|NLM_F_ROOT. Thus, if NLM_F_EXCL or
> >> > NLM_F_REPLACE flag is set, genetlink believes that you're
> >> > requesting a dump and it calls the .dumpit callback.
> >> > 
> >> > The solution that I propose is to refine this checking to
> >> > make it stricter:
> >> > 
> >> > 	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP)
> >> > 
> >> > And given the combination NLM_F_REPLACE and NLM_F_EXCL does
> >> > not make sense to me, it removes the ambiguity.
> >> > 
> >> > There was a patch that tried to fix this some time ago (0ab03c2
> >> > netlink: test for all flags of the NLM_F_DUMP composite) but it
> >> > tried to resolve this ambiguity in *all* existing netlink subsystems,
> >> > not only genetlink. That patch was reverted since it broke iproute2,
> >> > which is using NLM_F_ROOT to request the dump of the routing cache.
> >> > 
> >> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> >> 
> >> Yes, I remember that old attempt to fix this.
> >> 
> >> Ok, let's see what happens when we limit the scope of this change
> >> to just genetlink users.
> >>
> >> I honestly can't believe that NLM_F_EXCL and NLM_F_REPLACE are
> >> completely unusable in normal rtnetlink interfaces.
> > 
> > I guess you mean 'genetlink' instead of 'rtnetlink'.
> 
> I really meant 'rtnetlink' and netlink in general.  As you stated, we
> tried to make the same check for all netlink users, and it had to
> be reverted because iproute2 uses NLM_F_ROOT to get a dump.

Oh I see, sorry.

> Therefore I don't see how NLM_F_REPLACE and NLM_F_EXCL can be used
> at all, in those places, because the check is still "& NLM_F_DUMP"

The kind = type&3; is doing the magic there for rtnetlink. kind == 2
means that this is a get command, and you can only set NLM_F_DUMP
using the get command.

Since it doesn't make sense to use NLM_F_EXCL or NLM_F_REPLACE for get
commands, there is no room for ambiguity and rtnetlink is fine.

The old patch was too ambicious IMO, as it was trying to enforce
something in all netlink subsystems that we only needed to fix
genetlink.

Re: [PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On Thu, Aug 01, 2013 at 02:37:10AM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jul 31, 2013 at 05:03:48PM -0700, David Miller wrote:
[...]
> > Therefore I don't see how NLM_F_REPLACE and NLM_F_EXCL can be used
> > at all, in those places, because the check is still "& NLM_F_DUMP"
> 
> The kind = type&3; is doing the magic there for rtnetlink. kind == 2
> means that this is a get command, and you can only set NLM_F_DUMP
> using the get command.
> 
> Since it doesn't make sense to use NLM_F_EXCL or NLM_F_REPLACE for get
> commands, there is no room for ambiguity and rtnetlink is fine.

I had re-read what I wrote to get your point. We can fix in a
different way by checking for: ops->flags & GENL_CMD_CAP_DUMP, which
means we have a .dumpit callback, so only in that case genetlink
should interpret the flags as NLM_F_DUMP.

Please, see patch attached.

[-- Attachment #2: 0001-genetlink-interpret-NLM_F_DUMP-if-GENL_CMD_CAP_DUMP-.patch --]
[-- Type: text/x-diff, Size: 1284 bytes --]

>From 0536ae81c430d007a81dbdf2989b736f4f5057f1 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 1 Aug 2013 03:32:11 +0200
Subject: [PATCH] genetlink: interpret NLM_F_DUMP if GENL_CMD_CAP_DUMP flag is
 set

This patch reverts (e1ee367 genetlink: fix usage of NLM_F_EXCL
or NLM_F_REPLACE) to fix the possible ambiguity for non-get
commands in a different way. Basically, we assume that genetlink
should only interpret the NLM_F_DUMP flags if the .dumpit callback
is set, which is the common case for getoperation.

This approach is similar to what rtnetlink does to resolve this
ambiguity.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netlink/genetlink.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 512718a..d034728 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -571,7 +571,8 @@ static int genl_family_rcv_msg(struct genl_family *family,
 	    !capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
+	if ((ops->flags & GENL_CMD_CAP_DUMP) &&
+	    nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = ops->dumpit,
 			.done = ops->done,
-- 
1.7.10.4

Re: [PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 1 Aug 2013 04:00:49 +0200

> On Thu, Aug 01, 2013 at 02:37:10AM +0200, Pablo Neira Ayuso wrote:
>> On Wed, Jul 31, 2013 at 05:03:48PM -0700, David Miller wrote:
> [...]
>> > Therefore I don't see how NLM_F_REPLACE and NLM_F_EXCL can be used
>> > at all, in those places, because the check is still "& NLM_F_DUMP"
>> 
>> The kind = type&3; is doing the magic there for rtnetlink. kind == 2
>> means that this is a get command, and you can only set NLM_F_DUMP
>> using the get command.
>> 
>> Since it doesn't make sense to use NLM_F_EXCL or NLM_F_REPLACE for get
>> commands, there is no room for ambiguity and rtnetlink is fine.
> 
> I had re-read what I wrote to get your point. We can fix in a
> different way by checking for: ops->flags & GENL_CMD_CAP_DUMP, which
> means we have a .dumpit callback, so only in that case genetlink
> should interpret the flags as NLM_F_DUMP.
> 
> Please, see patch attached.

Thanks for the RFC patch, I'll think about this some more and reply.

Re: [PATCH] genetlink: fix usage of NLM_F_EXCL or NLM_F_REPLACE

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 1 Aug 2013 04:00:49 +0200

> @@ -571,7 +571,8 @@ static int genl_family_rcv_msg(struct genl_family *family,
>  	    !capable(CAP_NET_ADMIN))
>  		return -EPERM;
>  
> -	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
> +	if ((ops->flags & GENL_CMD_CAP_DUMP) &&
> +	    nlh->nlmsg_flags & NLM_F_DUMP) {
>  		struct netlink_dump_control c = {
>  			.dump = ops->dumpit,
>  			.done = ops->done,

Sorry for taking so long to get back to you on this, it looks perfect!

I did some auditing of other uses, and briefly crypto does the same:

crypto/crypto_user.c:

	if ((type == (CRYPTO_MSG_GETALG - CRYPTO_MSG_BASE) &&
	    (nlh->nlmsg_flags & NLM_F_DUMP))) {

"If this is a GET command, test dump flag"

Same thing for net/xfrm/xfrm_user.c:

	if ((type == (XFRM_MSG_GETSA - XFRM_MSG_BASE) ||
	     type == (XFRM_MSG_GETPOLICY - XFRM_MSG_BASE)) &&
	    (nlh->nlmsg_flags & NLM_F_DUMP)) {

Similarly all of the netfilter stuff performs this NLM_F_DUMP bit
test in contexts where we are processing some GET command.

All the "diag" modules are implicitly processing GET commands.

Pablo, could you please retest and resubmit this patch?  I will
apply it and push to -stable as well.

Thanks!