Unlike do requests, dump genetlink requests now perform strict validation
by default even if the genetlink family does not set policy and maxtype
because it does validation and parsing on its own (e.g. because it wants to
allow different message format for different commands). While the null
policy will be ignored, maxtype (which would be zero) is still checked so
that any attribute will fail validation.
The solution is to only call __nla_validate() from genl_family_rcv_msg()
if family->maxtype is set.
Fixes: ef6243acb478 ("genetlink: optionally validate strictly/dumps")
Signed-off-by: Michal Kubecek <[email protected]>
---
net/netlink/genetlink.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 72668759cd2b..9814d6dbd2d6 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -537,21 +537,25 @@ static int genl_family_rcv_msg(const struct genl_family *family,
return -EOPNOTSUPP;
if (!(ops->validate & GENL_DONT_VALIDATE_DUMP)) {
- unsigned int validate = NL_VALIDATE_STRICT;
int hdrlen = GENL_HDRLEN + family->hdrsize;
- if (ops->validate & GENL_DONT_VALIDATE_DUMP_STRICT)
- validate = NL_VALIDATE_LIBERAL;
-
if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
return -EINVAL;
- rc = __nla_validate(nlmsg_attrdata(nlh, hdrlen),
- nlmsg_attrlen(nlh, hdrlen),
- family->maxattr, family->policy,
- validate, extack);
- if (rc)
- return rc;
+ if (family->maxattr) {
+ unsigned int validate = NL_VALIDATE_STRICT;
+
+ if (ops->validate &
+ GENL_DONT_VALIDATE_DUMP_STRICT)
+ validate = NL_VALIDATE_LIBERAL;
+ rc = __nla_validate(nlmsg_attrdata(nlh, hdrlen),
+ nlmsg_attrlen(nlh, hdrlen),
+ family->maxattr,
+ family->policy,
+ validate, extack);
+ if (rc)
+ return rc;
+ }
}
if (!family->parallel_ops) {
--
2.21.0
On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> Unlike do requests, dump genetlink requests now perform strict validation
> by default even if the genetlink family does not set policy and maxtype
> because it does validation and parsing on its own (e.g. because it wants to
> allow different message format for different commands). While the null
> policy will be ignored, maxtype (which would be zero) is still checked so
> that any attribute will fail validation.
>
> The solution is to only call __nla_validate() from genl_family_rcv_msg()
> if family->maxtype is set.
D'oh. Which family was it that you found this on? I checked only ones
with policy I guess.
Reviewed-by: Johannes Berg <[email protected]>
johannes
On Thu, May 02, 2019 at 02:51:33PM +0200, Johannes Berg wrote:
> On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > Unlike do requests, dump genetlink requests now perform strict validation
> > by default even if the genetlink family does not set policy and maxtype
> > because it does validation and parsing on its own (e.g. because it wants to
> > allow different message format for different commands). While the null
> > policy will be ignored, maxtype (which would be zero) is still checked so
> > that any attribute will fail validation.
> >
> > The solution is to only call __nla_validate() from genl_family_rcv_msg()
> > if family->maxtype is set.
>
> D'oh. Which family was it that you found this on? I checked only ones
> with policy I guess.
It was with my ethtool netlink series (still work in progress).
Michal
> Reviewed-by: Johannes Berg <[email protected]>
>
> johannes
>
On Thu, 2019-05-02 at 15:10 +0200, Michal Kubecek wrote:
> On Thu, May 02, 2019 at 02:51:33PM +0200, Johannes Berg wrote:
> > On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > > Unlike do requests, dump genetlink requests now perform strict validation
> > > by default even if the genetlink family does not set policy and maxtype
> > > because it does validation and parsing on its own (e.g. because it wants to
> > > allow different message format for different commands). While the null
> > > policy will be ignored, maxtype (which would be zero) is still checked so
> > > that any attribute will fail validation.
> > >
> > > The solution is to only call __nla_validate() from genl_family_rcv_msg()
> > > if family->maxtype is set.
> >
> > D'oh. Which family was it that you found this on? I checked only ones
> > with policy I guess.
>
> It was with my ethtool netlink series (still work in progress).
Then you should probably *have* a policy to get all the other goodies
like automatic policy export (once I repost those patches)
johannes
On Thu, May 02, 2019 at 03:13:00PM +0200, Johannes Berg wrote:
> On Thu, 2019-05-02 at 15:10 +0200, Michal Kubecek wrote:
> > On Thu, May 02, 2019 at 02:51:33PM +0200, Johannes Berg wrote:
> > > On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > > > Unlike do requests, dump genetlink requests now perform strict validation
> > > > by default even if the genetlink family does not set policy and maxtype
> > > > because it does validation and parsing on its own (e.g. because it wants to
> > > > allow different message format for different commands). While the null
> > > > policy will be ignored, maxtype (which would be zero) is still checked so
> > > > that any attribute will fail validation.
> > > >
> > > > The solution is to only call __nla_validate() from genl_family_rcv_msg()
> > > > if family->maxtype is set.
> > >
> > > D'oh. Which family was it that you found this on? I checked only ones
> > > with policy I guess.
> >
> > It was with my ethtool netlink series (still work in progress).
>
> Then you should probably *have* a policy to get all the other goodies
> like automatic policy export (once I repost those patches)
Wouldn't it mean effecitvely ending up with only one command (in
genetlink sense) and having to distinguish actual commands with
atributes? Even if I wanted to have just "get" and "set" command, common
policy wouldn't allow me to say which attributes are allowed for each of
them.
Michal
On 5/2/19 7:32 AM, Michal Kubecek wrote:
> Wouldn't it mean effecitvely ending up with only one command (in
> genetlink sense) and having to distinguish actual commands with
> atributes? Even if I wanted to have just "get" and "set" command, common
> policy wouldn't allow me to say which attributes are allowed for each of
> them.
yes, I have been stuck on that as well.
There are a number of RTA attributes that are only valid for GET
requests or only used in the response or only valid in NEW requests.
Right now there is no discriminator when validating policies and the
patch set to expose the policies to userspace
On Thu, 2019-05-02 at 07:36 -0600, David Ahern wrote:
> On 5/2/19 7:32 AM, Michal Kubecek wrote:
> > Wouldn't it mean effecitvely ending up with only one command (in
> > genetlink sense) and having to distinguish actual commands with
> > atributes? Even if I wanted to have just "get" and "set" command, common
> > policy wouldn't allow me to say which attributes are allowed for each of
> > them.
>
> yes, I have been stuck on that as well.
>
> There are a number of RTA attributes that are only valid for GET
> requests or only used in the response or only valid in NEW requests.
> Right now there is no discriminator when validating policies and the
> patch set to expose the policies to userspace
Yeah. As I've been discussing with Pablo in various threads recently,
this is definitely something we're missing.
As I said there though, I think it's something we should treat as
orthogonal to the policies.
I haven't looked at your ethtool patches really now (if you have a git
tree that'd be nice), but I saw e.g.
+When appropriate, network device is identified by a nested attribute named
+ETHA_*_DEV. This attribute can contain
+
+ ETHA_DEV_INDEX (u32) device ifindex
+ ETHA_DEV_NAME (string) device name
Presumably, this is valid for each and every command, right?
I'm not sure I understand the "ETHA_*_DEV" part, but splitting the
policy per command means that things like this that are available/valid
for each command need to be stated over and over again. This opens up
the very easy possibility that you have one command that takes an
ETHA_DEV_INDEX as u32, and another that - for some reason - takes a u64
for example, or similar confusion between the same attribute stated in
different policies.
This is why I believe that when we have a flat namespace for attributes,
like ETHA_*, we should also have a flat policy for those attributes, and
that's why I made the genetlink to have a single policy.
At the same time, I do realize that this is not ideal. So far I've sort
of pushed this to be something that we should treat orthogonally to the
validation for the above reasons, i.e. *not* state this specifically in
the policy.
If we were able to express this in C, I'd probably say we should have
something like
static const struct genl_ops ops[] = {
{
.cmd = MY_CMD,
.doit = my_cmd_doit,
.valid_attrs = { MY_ATTR_A, MY_ATTR_B },
},
...
};
However, there's no way to express this in C code, except for
static const u16 my_cmd_valid_attrs[] = { MY_ATTR_A, MY_ATTR_B, 0 };
static const struct genl_ops ops[] = {
{
.cmd = MY_CMD,
.doit = my_cmd_doit,
.valid_attrs = my_cmd_valid_attrs,
},
...
};
which is clearly ugly to write. We could generate this code from a
domain-specific language like Pablo suggested, but I'm not really sure
that's ideal either.
Regardless, I think we should solve this problem orthogonally from the
policy, given that otherwise we can end up with the same attribute
meaning different things in different commands.
johannes