From: Johannes Berg <[email protected]>
In some situations some netlink attributes may be used for output
only (kernel->userspace) or may be reserved for future use. It's
then helpful to be able to prevent userspace from using them in
messages sent to the kernel, since they'd otherwise be ignored and
any future will become impossible if this happens.
Add NLA_REJECT to the policy which does nothing but reject (with
EINVAL) validation of any messages containing this attribute.
Allow for returning a specific extended ACK error message in the
validation_data pointer.
While at it fix the indentation of NLA_BITFIELD32 and describe the
validation_data pointer for it.
The specific case I have in mind now is a shared nested attribute
containing request/response data, and it would be pointless and
potentially confusing to have userspace include response data in
the messages that actually contain a request.
Signed-off-by: Johannes Berg <[email protected]>
---
include/net/netlink.h | 6 +++++-
lib/nlattr.c | 22 +++++++++++++++-------
2 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0c154f98e987..04e40fcc70d6 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -180,6 +180,7 @@ enum {
NLA_S32,
NLA_S64,
NLA_BITFIELD32,
+ NLA_REJECT,
__NLA_TYPE_MAX,
};
@@ -208,7 +209,10 @@ enum {
* NLA_MSECS Leaving the length field zero will verify the
* given type fits, using it verifies minimum length
* just like "All other"
- * NLA_BITFIELD32 A 32-bit bitmap/bitselector attribute
+ * NLA_BITFIELD32 A 32-bit bitmap/bitselector attribute, validation
+ * data must point to a u32 value of valid flags
+ * NLA_REJECT Reject this attribute, validation data may point
+ * to a string to report as the error in extended ACK.
* All other Minimum length of attribute payload
*
* Example:
diff --git a/lib/nlattr.c b/lib/nlattr.c
index e335bcafa9e4..56e0aae5cf23 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,7 +69,8 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
}
static int validate_nla(const struct nlattr *nla, int maxtype,
- const struct nla_policy *policy)
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
{
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
@@ -87,6 +88,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
}
switch (pt->type) {
+ case NLA_REJECT:
+ if (pt->validation_data && extack)
+ extack->_msg = pt->validation_data;
+ return -EINVAL;
+
case NLA_FLAG:
if (attrlen > 0)
return -ERANGE;
@@ -180,11 +186,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
int rem;
nla_for_each_attr(nla, head, len, rem) {
- int err = validate_nla(nla, maxtype, policy);
+ int err = validate_nla(nla, maxtype, policy, extack);
if (err < 0) {
- if (extack)
- extack->bad_attr = nla;
+ NL_SET_BAD_ATTR(extack, nla);
return err;
}
}
@@ -251,10 +256,13 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
if (type > 0 && type <= maxtype) {
if (policy) {
- err = validate_nla(nla, maxtype, policy);
+ err = validate_nla(nla, maxtype, policy,
+ extack);
if (err < 0) {
- NL_SET_ERR_MSG_ATTR(extack, nla,
- "Attribute failed policy validation");
+ NL_SET_BAD_ATTR(extack, nla);
+ if (extack && !extack->_msg)
+ NL_SET_ERR_MSG(extack,
+ "Attribute failed policy validation");
goto errout;
}
}
--
2.14.4
On Thu, Sep 13, 2018 at 10:46:02AM +0200, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> In some situations some netlink attributes may be used for output
> only (kernel->userspace) or may be reserved for future use. It's
> then helpful to be able to prevent userspace from using them in
> messages sent to the kernel, since they'd otherwise be ignored and
> any future will become impossible if this happens.
I don't follow, what's the issue with simply ignoring such attributes?
>
> Add NLA_REJECT to the policy which does nothing but reject (with
> EINVAL) validation of any messages containing this attribute.
> Allow for returning a specific extended ACK error message in the
> validation_data pointer.
This has potential to create confusion because we can't use it on
{output,reserved} attributes that are already there (as we must always
ignore them now) and we will end up with a mix of it.
>
> While at it fix the indentation of NLA_BITFIELD32 and describe the
> validation_data pointer for it.
>
> The specific case I have in mind now is a shared nested attribute
> containing request/response data, and it would be pointless and
> potentially confusing to have userspace include response data in
> the messages that actually contain a request.
Would be nice to see some patches for it too. Maybe it helps.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> include/net/netlink.h | 6 +++++-
> lib/nlattr.c | 22 +++++++++++++++-------
> 2 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 0c154f98e987..04e40fcc70d6 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -180,6 +180,7 @@ enum {
> NLA_S32,
> NLA_S64,
> NLA_BITFIELD32,
> + NLA_REJECT,
> __NLA_TYPE_MAX,
> };
>
> @@ -208,7 +209,10 @@ enum {
> * NLA_MSECS Leaving the length field zero will verify the
> * given type fits, using it verifies minimum length
> * just like "All other"
> - * NLA_BITFIELD32 A 32-bit bitmap/bitselector attribute
> + * NLA_BITFIELD32 A 32-bit bitmap/bitselector attribute, validation
> + * data must point to a u32 value of valid flags
> + * NLA_REJECT Reject this attribute, validation data may point
> + * to a string to report as the error in extended ACK.
> * All other Minimum length of attribute payload
> *
> * Example:
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index e335bcafa9e4..56e0aae5cf23 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -69,7 +69,8 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
> }
>
> static int validate_nla(const struct nlattr *nla, int maxtype,
> - const struct nla_policy *policy)
> + const struct nla_policy *policy,
> + struct netlink_ext_ack *extack)
> {
> const struct nla_policy *pt;
> int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
> @@ -87,6 +88,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> }
>
> switch (pt->type) {
> + case NLA_REJECT:
> + if (pt->validation_data && extack)
> + extack->_msg = pt->validation_data;
> + return -EINVAL;
> +
> case NLA_FLAG:
> if (attrlen > 0)
> return -ERANGE;
> @@ -180,11 +186,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
> int rem;
>
> nla_for_each_attr(nla, head, len, rem) {
> - int err = validate_nla(nla, maxtype, policy);
> + int err = validate_nla(nla, maxtype, policy, extack);
>
> if (err < 0) {
> - if (extack)
> - extack->bad_attr = nla;
> + NL_SET_BAD_ATTR(extack, nla);
> return err;
> }
> }
> @@ -251,10 +256,13 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
>
> if (type > 0 && type <= maxtype) {
> if (policy) {
> - err = validate_nla(nla, maxtype, policy);
> + err = validate_nla(nla, maxtype, policy,
> + extack);
> if (err < 0) {
> - NL_SET_ERR_MSG_ATTR(extack, nla,
> - "Attribute failed policy validation");
> + NL_SET_BAD_ATTR(extack, nla);
> + if (extack && !extack->_msg)
> + NL_SET_ERR_MSG(extack,
> + "Attribute failed policy validation");
> goto errout;
> }
> }
> --
> 2.14.4
>
On Thu, 2018-09-13 at 14:24 +0200, Michal Kubecek wrote:
> On Thu, Sep 13, 2018 at 02:16:06PM +0200, Johannes Berg wrote:
> > On Thu, 2018-09-13 at 14:12 +0200, Michal Kubecek wrote:
> > > On Thu, Sep 13, 2018 at 02:02:53PM +0200, Johannes Berg wrote:
> > > > On Thu, 2018-09-13 at 13:58 +0200, Michal Kubecek wrote:
> > > >
> > > > > The code looks correct to me but I have some doubts. Having a special
> > > > > policy for MAC addresses may lead to adding one for IPv4 address (maybe
> > > > > not, we can use NLA_U32 for them), IPv6 addresses and other data types
> > > > > with fixed length. Wouldn't it be more helpful to add a variant of
> > > > > NLA_BINARY (NLA_BINARY_EXACT?) which would fail/warn if attribute length
> > > > > isn't equal to .len?
> > > >
> > > > Yeah, I guess we could do that, and then
> > > >
> > > > #define NLA_ETH_ADDR .len = ETH_ALEN, .type = NLA_BINARY_EXACT
> > > > #define NLA_IP6_ADDR .len = 16, .type = NLA_BINARY_EXACT
> > > >
> > > > or so?
> > >
> > > Maybe rather
> > >
> > > #define NLA_ETH_ADDR NLA_BINARY_EXACT, .len = ETH_ALEN
> > > #define NLA_IP6_ADDR NLA_BINARY_EXACT, .len = sizeof(struct in6_addr)
> > >
> > > so that one could write
> > >
> > > { .type = NLA_ETH_ADDR }
> >
> > Yeah, that's possible. I considered it for a second, but it was slightly
> > too magical for my taste :-)
> >
> > Better pick a different "namespace", perhaps NLA_POLICY_ETH_ADDR or so?
>
> Right, that sounds better. I'm afraid anything too tricky would
> inevitably trick people into using it in an unexpected way and ending up
> with very confusing error messages.
Right.
Then again though, we already have NLA_MSECS which is basically
equivalent to NLA_U64 afaict, so why not have more types?
It doesn't really cost us that much, just a few bytes in the validation?
Also, with .type = NLA_ETH_ADDR_COMPAT we could get a warning, which is
not true for just checking .len.
OTOH, you could argue that adding two types for ethernet addresses, two
for IPv6 addresses, and possibly more quickly adds up to make that "just
a few bytes" matter ...
johannes
From: Johannes Berg <[email protected]>
Commonly, ethernet addresses are just using a policy of
{ .len = ETH_ALEN }
which leaves userspace free to send more data than it should,
which may hide bugs.
Introduce NLA_ETH_ADDR which checks for exact size, and rejects
the attribute if the length isn't ETH_ALEN.
Also add NLA_ETH_ADDR_COMPAT which can be used in place of the
pure length policy, but will, in addition, trigger a netlink
attribute warning if the passed value is too long.
Signed-off-by: Johannes Berg <[email protected]>
---
include/net/netlink.h | 4 ++++
lib/nlattr.c | 8 ++++++++
2 files changed, 12 insertions(+)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 04e40fcc70d6..1139163c0db0 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -181,6 +181,8 @@ enum {
NLA_S64,
NLA_BITFIELD32,
NLA_REJECT,
+ NLA_ETH_ADDR,
+ NLA_ETH_ADDR_COMPAT,
__NLA_TYPE_MAX,
};
@@ -213,6 +215,8 @@ enum {
* data must point to a u32 value of valid flags
* NLA_REJECT Reject this attribute, validation data may point
* to a string to report as the error in extended ACK.
+ * NLA_ETH_ADDR Ethernet address, rejected if not exactly 6 octets.
+ * NLA_ETH_ADDR_COMPAT Ethernet address, only warns if not exactly 6 octets.
* All other Minimum length of attribute payload
*
* Example:
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 56e0aae5cf23..b7c519f6e12a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -29,6 +29,8 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
[NLA_S16] = sizeof(s16),
[NLA_S32] = sizeof(s32),
[NLA_S64] = sizeof(s64),
+ [NLA_ETH_ADDR] = ETH_ALEN,
+ [NLA_ETH_ADDR_COMPAT] = ETH_ALEN,
};
static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
@@ -42,6 +44,7 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
[NLA_S16] = sizeof(s16),
[NLA_S32] = sizeof(s32),
[NLA_S64] = sizeof(s64),
+ [NLA_ETH_ADDR_COMPAT] = ETH_ALEN,
};
static int validate_nla_bitfield32(const struct nlattr *nla,
@@ -93,6 +96,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
extack->_msg = pt->validation_data;
return -EINVAL;
+ case NLA_ETH_ADDR:
+ if (attrlen != ETH_ALEN)
+ return -ERANGE;
+ break;
+
case NLA_FLAG:
if (attrlen > 0)
return -ERANGE;
--
2.14.4
On Thu, 2018-09-13 at 15:59 -0700, David Miller wrote:
> From: Johannes Berg <[email protected]>
> Date: Thu, 13 Sep 2018 10:46:02 +0200
>
> > + NL_SET_BAD_ATTR(extack, nla);
> > + if (extack && !extack->_msg)
> > + NL_SET_ERR_MSG(extack,
> > + "Attribute failed policy validation");
>
> Given the lively discussion that resulted from this conditional I am
> pretty sure we want to override existing messages.
>
> If we have an existing message, and we continued to process and
> parse anyways, then the existing message was informational or
> a warning.
>
> The message should be overridden when the action will be to fail, as
> it will be here when we return -EINVAL.
Not just -EINVAL, but yeah, I've just reworked the patch to do this.
johannes
On Tue, 2018-09-18 at 08:55 -0400, Jamal Hadi Salim wrote:
> Execute permission kind of thing? i.e if i understood you correctly
> if acl is "rwx" then attribute can only be written to (or read from) if
> the "thing executing" is complete
But it's not an attribute that you're executing, it's some kind of
command, and then you get the return value of that command in that
attribute?
Say you want to scan for wifi networks - you trigger a scan, later you
get a notification giving you some data about the scan (let's say the
time it took) - there's no way you can set that time attribute.
(NB: it doesn't work this way, we don't have that attribute now, but I
didn't want to pick a more complicated example)
> > What would the practical difference be though? Hopefully you wouldn't
> > have write-only attributes, and then NLA_REJECT is basically equivalent?
> >
>
> If ACL says "-w-" then reading should get explicit permission denied
> code possibly with an extack which is more descriptive that reading
> is not allowed.
Perhaps. But NLA_REJECT comes with an extack string to tell you, so ...
I dunno. I think we already bloated the policies too much by including
the validation_data pointer, and would hate to add more to that :-)
johannes
On Mon, Sep 17, 2018 at 11:38:52AM +0200, Johannes Berg wrote:
> On Thu, 2018-09-13 at 18:58 -0300, Marcelo Ricardo Leitner wrote:
>
> > Then I ask my first question again: why reject these? They are not
> > hurting anything, are they? It's different from your example I think.
> > In there, the extra information which was ignored leads to a
> > different behavior.
>
> So in one case I was thinking of, there are some fields that simply
> cannot be used for input, they're only used for output. But it may not
> always be obvious to somebody using the API. Thus, I think it makes
> sense to instruct the kernel to reject that, so that whoever gets
> confused has immediate feedback that their usage is wrong. If we ignore
> that, they may not realize their error immediately.
>
> I think the ethtool case is similar: you can read and write some fields,
> and only read others - but if you try to write the read-only fields
> would you prefer to be told "sorry, this is not possible" vs. it being
> silently ignored? I'd definitely prefer the former.
See below.
>
> > Maybe it would be better to have NLA_IGNORE instead? </idea>
>
> I don't think so, it doesn't give any feedback to the application author
> that they're doing something wrong.
Yes, it would have to have some other ways to signal return values.
In some cases there may be other ways to tell the application that it
couldn't be handled at the time. For example, when changing several
ethtool offloadings at once, we could have a feedback for each of the
offloading that was request and it could include "ack, nack and
ignored" and let the application decide if that "ignored" should be an
error or not. It all boils down to what one is trying to do.
That said, I'm now liking this patch. Just too bad we cannot flag
current output-only fields as so, but in the long term I think this
patch will help us.
Thanks,
Marcelo
On Thu, 2018-09-13 at 18:58 -0300, Marcelo Ricardo Leitner wrote:
> > It would be easier and IMHO cleaner if I could simply list these "read
> > only attributes" with NLA_REJECT policy for "set" request.
>
> Not that I'm against this. Point was fields that are considered output
> only today are probably being silently ignored, and we can't change
> them to be NLA_REJECT as it would break user applications.
Indeed.
> Then we
> will have fields that are rejected, and those old that are not. In the
> long run, nearly all output fields would be marked as NLA_REJECT,
> okay.
Perhaps, yes, though I assume it would only really be true for new
families that bother to mark as such.
> Then I ask my first question again: why reject these? They are not
> hurting anything, are they? It's different from your example I think.
> In there, the extra information which was ignored leads to a
> different behavior.
So in one case I was thinking of, there are some fields that simply
cannot be used for input, they're only used for output. But it may not
always be obvious to somebody using the API. Thus, I think it makes
sense to instruct the kernel to reject that, so that whoever gets
confused has immediate feedback that their usage is wrong. If we ignore
that, they may not realize their error immediately.
I think the ethtool case is similar: you can read and write some fields,
and only read others - but if you try to write the read-only fields
would you prefer to be told "sorry, this is not possible" vs. it being
silently ignored? I'd definitely prefer the former.
> Maybe it would be better to have NLA_IGNORE instead? </idea>
I don't think so, it doesn't give any feedback to the application author
that they're doing something wrong.
johannes
On 2018-09-18 8:57 a.m., Johannes Berg wrote:
> On Tue, 2018-09-18 at 08:55 -0400, Jamal Hadi Salim wrote:
>
>> Execute permission kind of thing? i.e if i understood you correctly
>> if acl is "rwx" then attribute can only be written to (or read from) if
>> the "thing executing" is complete
>
> But it's not an attribute that you're executing, it's some kind of
> command, and then you get the return value of that command in that
> attribute?
>
> Say you want to scan for wifi networks - you trigger a scan, later you
> get a notification giving you some data about the scan (let's say the
> time it took) - there's no way you can set that time attribute.
>
Not very familiar with how wifi scan gets initiated. I am guessing
you issue some GET or SET to start a scan - and you get an async
response when it is complete (and it would include the time it took)?
Or maybe you get an immediate response and event notification later
and the time spent is in that notification?
I would still see that as a read-only attribute.
And the utility of "execute" bit is only in blocking another scan
from being initiated when one is in progress, if that is a desired
goal.
Note in most net devices stats can only be read but not written to
for example.
> (NB: it doesn't work this way, we don't have that attribute now, but I
> didn't want to pick a more complicated example)
>
>>> What would the practical difference be though? Hopefully you wouldn't
>>> have write-only attributes, and then NLA_REJECT is basically equivalent?
>>>
>>
>> If ACL says "-w-" then reading should get explicit permission denied
>> code possibly with an extack which is more descriptive that reading
>> is not allowed.
>
> Perhaps. But NLA_REJECT comes with an extack string to tell you, so ...
>
> I dunno. I think we already bloated the policies too much by including
> the validation_data pointer, and would hate to add more to that :-)
Your mileage may vary. NLA_REJECT may work acls offer more fine grained
controls.
cheers,
jamal
On Thu, Sep 13, 2018 at 02:16:06PM +0200, Johannes Berg wrote:
> On Thu, 2018-09-13 at 14:12 +0200, Michal Kubecek wrote:
> > On Thu, Sep 13, 2018 at 02:02:53PM +0200, Johannes Berg wrote:
> > > On Thu, 2018-09-13 at 13:58 +0200, Michal Kubecek wrote:
> > >
> > > > The code looks correct to me but I have some doubts. Having a special
> > > > policy for MAC addresses may lead to adding one for IPv4 address (maybe
> > > > not, we can use NLA_U32 for them), IPv6 addresses and other data types
> > > > with fixed length. Wouldn't it be more helpful to add a variant of
> > > > NLA_BINARY (NLA_BINARY_EXACT?) which would fail/warn if attribute length
> > > > isn't equal to .len?
> > >
> > > Yeah, I guess we could do that, and then
> > >
> > > #define NLA_ETH_ADDR .len = ETH_ALEN, .type = NLA_BINARY_EXACT
> > > #define NLA_IP6_ADDR .len = 16, .type = NLA_BINARY_EXACT
> > >
> > > or so?
> >
> > Maybe rather
> >
> > #define NLA_ETH_ADDR NLA_BINARY_EXACT, .len = ETH_ALEN
> > #define NLA_IP6_ADDR NLA_BINARY_EXACT, .len = sizeof(struct in6_addr)
> >
> > so that one could write
> >
> > { .type = NLA_ETH_ADDR }
>
> Yeah, that's possible. I considered it for a second, but it was slightly
> too magical for my taste :-)
>
> Better pick a different "namespace", perhaps NLA_POLICY_ETH_ADDR or so?
Right, that sounds better. I'm afraid anything too tricky would
inevitably trick people into using it in an unexpected way and ending up
with very confusing error messages.
Michal Kubecek
On Thu, 2018-09-13 at 22:39 +0200, Michal Kubecek wrote:
> > What about
> > #define NLA_FIELD_ETH_ADDR { .type = NLA_BINARY_EXACT, .len = ETH_ALEN }
> >
> > Or even
> > #define NLA_FIELD_BINARY_EXACT(_len) { .type = NLA_BINARY_EXACT, .len = (_len) }
> > #define NLA_FIELD_ETH_ADDR NLA_FIELD_BINARY_EXACT(ETH_ALEN)
> >
> > So that one would just:
> > [MYADDR] = NLA_FIELD_ETH_ADDR,
>
> Yes, that's how I understoold Johannes' proposal above.
I was really thinking of
[MYADDR] = { NLA_FIELD_ETH_ADDR },
but it doesn't really matter much to me either way.
johannes
On Thu, Sep 13, 2018 at 02:02:53PM +0200, Johannes Berg wrote:
> On Thu, 2018-09-13 at 13:58 +0200, Michal Kubecek wrote:
>
> > The code looks correct to me but I have some doubts. Having a special
> > policy for MAC addresses may lead to adding one for IPv4 address (maybe
> > not, we can use NLA_U32 for them), IPv6 addresses and other data types
> > with fixed length. Wouldn't it be more helpful to add a variant of
> > NLA_BINARY (NLA_BINARY_EXACT?) which would fail/warn if attribute length
> > isn't equal to .len?
>
> Yeah, I guess we could do that, and then
>
> #define NLA_ETH_ADDR .len = ETH_ALEN, .type = NLA_BINARY_EXACT
> #define NLA_IP6_ADDR .len = 16, .type = NLA_BINARY_EXACT
>
> or so?
Maybe rather
#define NLA_ETH_ADDR NLA_BINARY_EXACT, .len = ETH_ALEN
#define NLA_IP6_ADDR NLA_BINARY_EXACT, .len = sizeof(struct in6_addr)
so that one could write
{ .type = NLA_ETH_ADDR }
as with other types.
Michal Kubecek
On Thu, Sep 13, 2018 at 01:25:15PM +0200, Johannes Berg wrote:
> On Thu, 2018-09-13 at 12:49 +0200, Michal Kubecek wrote:
>
> > > if (type > 0 && type <= maxtype) {
> > > if (policy) {
> > > - err = validate_nla(nla, maxtype, policy);
> > > + err = validate_nla(nla, maxtype, policy,
> > > + extack);
> > > if (err < 0) {
> > > - NL_SET_ERR_MSG_ATTR(extack, nla,
> > > - "Attribute failed policy validation");
> > > + NL_SET_BAD_ATTR(extack, nla);
> > > + if (extack && !extack->_msg)
> > > + NL_SET_ERR_MSG(extack,
> > > + "Attribute failed policy validation");
> > > goto errout;
> > > }
> > > }
> > > --
> >
> > Technically, this would change the outcome when nla_parse() is called
> > with extack->_msg already set nad validate_nla() fails on something else
> > than NLA_REJECT; it will preserve the previous message in such case.
> > But I don't think this is a serious problem.
>
> Yes, that's true. I looked at quite a few of the setters just now (there
> are ~500 already, wow!), and they all set & return, so this shouldn't be
> an issue.
In ethtool (work in progress) I sometimes use extack message for
non-fatal warnings but AFAICS never before parsing the userspace
request (actually always shortly before returning). So I don't have
a problem with it.
Michal Kubecek
On Thu, Sep 13, 2018 at 10:46:03AM +0200, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> Commonly, ethernet addresses are just using a policy of
> { .len = ETH_ALEN }
> which leaves userspace free to send more data than it should,
> which may hide bugs.
>
> Introduce NLA_ETH_ADDR which checks for exact size, and rejects
> the attribute if the length isn't ETH_ALEN.
>
> Also add NLA_ETH_ADDR_COMPAT which can be used in place of the
> pure length policy, but will, in addition, trigger a netlink
> attribute warning if the passed value is too long.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
The code looks correct to me but I have some doubts. Having a special
policy for MAC addresses may lead to adding one for IPv4 address (maybe
not, we can use NLA_U32 for them), IPv6 addresses and other data types
with fixed length. Wouldn't it be more helpful to add a variant of
NLA_BINARY (NLA_BINARY_EXACT?) which would fail/warn if attribute length
isn't equal to .len?
Michal Kubecek
> include/net/netlink.h | 4 ++++
> lib/nlattr.c | 8 ++++++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 04e40fcc70d6..1139163c0db0 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -181,6 +181,8 @@ enum {
> NLA_S64,
> NLA_BITFIELD32,
> NLA_REJECT,
> + NLA_ETH_ADDR,
> + NLA_ETH_ADDR_COMPAT,
> __NLA_TYPE_MAX,
> };
>
> @@ -213,6 +215,8 @@ enum {
> * data must point to a u32 value of valid flags
> * NLA_REJECT Reject this attribute, validation data may point
> * to a string to report as the error in extended ACK.
> + * NLA_ETH_ADDR Ethernet address, rejected if not exactly 6 octets.
> + * NLA_ETH_ADDR_COMPAT Ethernet address, only warns if not exactly 6 octets.
> * All other Minimum length of attribute payload
> *
> * Example:
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 56e0aae5cf23..b7c519f6e12a 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -29,6 +29,8 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
> [NLA_S16] = sizeof(s16),
> [NLA_S32] = sizeof(s32),
> [NLA_S64] = sizeof(s64),
> + [NLA_ETH_ADDR] = ETH_ALEN,
> + [NLA_ETH_ADDR_COMPAT] = ETH_ALEN,
> };
>
> static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
> @@ -42,6 +44,7 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
> [NLA_S16] = sizeof(s16),
> [NLA_S32] = sizeof(s32),
> [NLA_S64] = sizeof(s64),
> + [NLA_ETH_ADDR_COMPAT] = ETH_ALEN,
> };
>
> static int validate_nla_bitfield32(const struct nlattr *nla,
> @@ -93,6 +96,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> extack->_msg = pt->validation_data;
> return -EINVAL;
>
> + case NLA_ETH_ADDR:
> + if (attrlen != ETH_ALEN)
> + return -ERANGE;
> + break;
> +
> case NLA_FLAG:
> if (attrlen > 0)
> return -ERANGE;
> --
> 2.14.4
>
On Thu, Sep 13, 2018 at 04:20:14PM -0300, Marcelo Ricardo Leitner wrote:
> On Thu, Sep 13, 2018 at 02:05:54PM +0200, Michal Kubecek wrote:
> > On Thu, Sep 13, 2018 at 01:25:15PM +0200, Johannes Berg wrote:
> > > On Thu, 2018-09-13 at 12:49 +0200, Michal Kubecek wrote:
> > >
> > > > > if (type > 0 && type <= maxtype) {
> > > > > if (policy) {
> > > > > - err = validate_nla(nla, maxtype, policy);
> > > > > + err = validate_nla(nla, maxtype, policy,
> > > > > + extack);
> > > > > if (err < 0) {
> > > > > - NL_SET_ERR_MSG_ATTR(extack, nla,
> > > > > - "Attribute failed policy validation");
> > > > > + NL_SET_BAD_ATTR(extack, nla);
> > > > > + if (extack && !extack->_msg)
> > > > > + NL_SET_ERR_MSG(extack,
> > > > > + "Attribute failed policy validation");
> > > > > goto errout;
> > > > > }
> > > > > }
> > > > > --
> > > >
> > > > Technically, this would change the outcome when nla_parse() is called
> > > > with extack->_msg already set nad validate_nla() fails on something else
> > > > than NLA_REJECT; it will preserve the previous message in such case.
> > > > But I don't think this is a serious problem.
> > >
> > > Yes, that's true. I looked at quite a few of the setters just now (there
> > > are ~500 already, wow!), and they all set & return, so this shouldn't be
> > > an issue.
> >
> > In ethtool (work in progress) I sometimes use extack message for
> > non-fatal warnings but AFAICS never before parsing the userspace
> > request (actually always shortly before returning). So I don't have
> > a problem with it.
>
> Considering we can only report 1 message, it should be okay to drop
> the previous message in favor of the new one, which is either a
> critical one or just another non-fatal one.
What I wanted to point out is that the code above does not behave like
this. It does not distinguish between extack->_msg set by NLA_REJECT
branch and extack->_msg which had been set before nla_parse() was
called.
Michal Kubecek
On Thu, Sep 13, 2018 at 02:16:06PM +0200, Johannes Berg wrote:
> On Thu, 2018-09-13 at 14:12 +0200, Michal Kubecek wrote:
> > On Thu, Sep 13, 2018 at 02:02:53PM +0200, Johannes Berg wrote:
> > > On Thu, 2018-09-13 at 13:58 +0200, Michal Kubecek wrote:
> > >
> > > > The code looks correct to me but I have some doubts. Having a special
> > > > policy for MAC addresses may lead to adding one for IPv4 address (maybe
> > > > not, we can use NLA_U32 for them), IPv6 addresses and other data types
> > > > with fixed length. Wouldn't it be more helpful to add a variant of
> > > > NLA_BINARY (NLA_BINARY_EXACT?) which would fail/warn if attribute length
> > > > isn't equal to .len?
> > >
> > > Yeah, I guess we could do that, and then
> > >
> > > #define NLA_ETH_ADDR .len = ETH_ALEN, .type = NLA_BINARY_EXACT
> > > #define NLA_IP6_ADDR .len = 16, .type = NLA_BINARY_EXACT
> > >
> > > or so?
> >
> > Maybe rather
> >
> > #define NLA_ETH_ADDR NLA_BINARY_EXACT, .len = ETH_ALEN
> > #define NLA_IP6_ADDR NLA_BINARY_EXACT, .len = sizeof(struct in6_addr)
> >
> > so that one could write
> >
> > { .type = NLA_ETH_ADDR }
>
> Yeah, that's possible. I considered it for a second, but it was slightly
> too magical for my taste :-)
>
> Better pick a different "namespace", perhaps NLA_POLICY_ETH_ADDR or so?
What about
#define NLA_FIELD_ETH_ADDR { .type = NLA_BINARY_EXACT, .len = ETH_ALEN }
Or even
#define NLA_FIELD_BINARY_EXACT(_len) { .type = NLA_BINARY_EXACT, .len = (_len) }
#define NLA_FIELD_ETH_ADDR NLA_FIELD_BINARY_EXACT(ETH_ALEN)
So that one would just:
[MYADDR] = NLA_FIELD_ETH_ADDR,
and if we change how we parse/validate it, users should be good
already.
Marcelo
On Thu, 2018-09-13 at 12:49 +0200, Michal Kubecek wrote:
> > if (type > 0 && type <= maxtype) {
> > if (policy) {
> > - err = validate_nla(nla, maxtype, policy);
> > + err = validate_nla(nla, maxtype, policy,
> > + extack);
> > if (err < 0) {
> > - NL_SET_ERR_MSG_ATTR(extack, nla,
> > - "Attribute failed policy validation");
> > + NL_SET_BAD_ATTR(extack, nla);
> > + if (extack && !extack->_msg)
> > + NL_SET_ERR_MSG(extack,
> > + "Attribute failed policy validation");
> > goto errout;
> > }
> > }
> > --
>
> Technically, this would change the outcome when nla_parse() is called
> with extack->_msg already set nad validate_nla() fails on something else
> than NLA_REJECT; it will preserve the previous message in such case.
> But I don't think this is a serious problem.
Yes, that's true. I looked at quite a few of the setters just now (there
are ~500 already, wow!), and they all set & return, so this shouldn't be
an issue.
johannes
On Thu, 2018-09-13 at 14:12 +0200, Michal Kubecek wrote:
> On Thu, Sep 13, 2018 at 02:02:53PM +0200, Johannes Berg wrote:
> > On Thu, 2018-09-13 at 13:58 +0200, Michal Kubecek wrote:
> >
> > > The code looks correct to me but I have some doubts. Having a special
> > > policy for MAC addresses may lead to adding one for IPv4 address (maybe
> > > not, we can use NLA_U32 for them), IPv6 addresses and other data types
> > > with fixed length. Wouldn't it be more helpful to add a variant of
> > > NLA_BINARY (NLA_BINARY_EXACT?) which would fail/warn if attribute length
> > > isn't equal to .len?
> >
> > Yeah, I guess we could do that, and then
> >
> > #define NLA_ETH_ADDR .len = ETH_ALEN, .type = NLA_BINARY_EXACT
> > #define NLA_IP6_ADDR .len = 16, .type = NLA_BINARY_EXACT
> >
> > or so?
>
> Maybe rather
>
> #define NLA_ETH_ADDR NLA_BINARY_EXACT, .len = ETH_ALEN
> #define NLA_IP6_ADDR NLA_BINARY_EXACT, .len = sizeof(struct in6_addr)
>
> so that one could write
>
> { .type = NLA_ETH_ADDR }
Yeah, that's possible. I considered it for a second, but it was slightly
too magical for my taste :-)
Better pick a different "namespace", perhaps NLA_POLICY_ETH_ADDR or so?
johannes
On Tue, 2018-09-18 at 09:12 -0400, Jamal Hadi Salim wrote:
> Not very familiar with how wifi scan gets initiated. I am guessing
> you issue some GET or SET to start a scan - and you get an async
> response when it is complete (and it would include the time it took)?
> Or maybe you get an immediate response and event notification later
> and the time spent is in that notification?
There isn't really a GET or SET. It's just an arbitrary generic netlink
command that you send down. Not everything is a GET/SET model :-)
But yes, you issue a command via generic netlink to start a scan, with
some attributes, and then eventually get an async notification when it's
done (or was aborted for some reason.)
For purposes of my example, that time attribute would be in the
notification, yes.
> I would still see that as a read-only attribute.
I suppose you could, but you never really get to see it anywhere else.
> And the utility of "execute" bit is only in blocking another scan
> from being initiated when one is in progress, if that is a desired
> goal.
Not sure I understand that. I'm not really talking about some sort of
generic "SET_VALUE" command with a "SCAN_NOW" attribute?
> > I dunno. I think we already bloated the policies too much by including
> > the validation_data pointer, and would hate to add more to that :-)
>
> Your mileage may vary. NLA_REJECT may work acls offer more fine grained
> controls.
Fair point. We do have padding in the policy (at least on 64-bit
platforms) that we could use for more bits ;-)
johannes
On Thu, Sep 13, 2018 at 11:27:42PM +0200, Michal Kubecek wrote:
> On Thu, Sep 13, 2018 at 04:30:04PM -0300, Marcelo Ricardo Leitner wrote:
> > On Thu, Sep 13, 2018 at 10:46:02AM +0200, Johannes Berg wrote:
> > > From: Johannes Berg <[email protected]>
> > >
> > > In some situations some netlink attributes may be used for output
> > > only (kernel->userspace) or may be reserved for future use. It's
> > > then helpful to be able to prevent userspace from using them in
> > > messages sent to the kernel, since they'd otherwise be ignored and
> > > any future will become impossible if this happens.
> >
> > I don't follow, what's the issue with simply ignoring such attributes?
>
> A bit artificial example but I can't come with somethin better at the
> moment: let's say newer kernel and iproute2 allow something like
>
> ip link del <condition1> <condition2>
>
> and you run new ip with older kernel which only supports <condition1>.
> You don't really want kernel to silently ignore the second condition.
> Or e.g. when adding a netfilter rule, you wouldn't want kernel to ignore
> parts of the rule it does not understand.
Oh, I thought these are different issues. How would NLA_REJECT protect
against this, by reserving <condition2> in advance?
This example looks more like a case for NLM_F_STRICT:
https://lwn.net/Articles/661266/
On which the user space would explicitly say "please reject this if
you don't get it all", but it was rejected back then.
>
> I must admit I'm not sure if there is really need for having reserved
> attributes with netlink. Maybe e.g. when we want to share part of
> (numeric) attribute types between different message types. Anyway, we
> have the same problem with attributes higher than maximum; NLA_REJECT
> wouldn't help with this but the discussion also touched the topic.
Yes, agree.
>
> > > Add NLA_REJECT to the policy which does nothing but reject (with
> > > EINVAL) validation of any messages containing this attribute.
> > > Allow for returning a specific extended ACK error message in the
> > > validation_data pointer.
> >
> > This has potential to create confusion because we can't use it on
> > {output,reserved} attributes that are already there (as we must always
> > ignore them now) and we will end up with a mix of it.
>
> We can return -EINVAL even now, we just need to add a check after
> nla_parse() wrapper returns, e.g. here: (lines 314-320)
>
> https://github.com/mkubecek/ethnl/blob/master/kernel/0019-ethtool-implement-SET_PARAMS-message.patch#L314
That's new code, it's okay. Won't break anyone's setup.
>
> It would be easier and IMHO cleaner if I could simply list these "read
> only attributes" with NLA_REJECT policy for "set" request.
Not that I'm against this. Point was fields that are considered output
only today are probably being silently ignored, and we can't change
them to be NLA_REJECT as it would break user applications. Then we
will have fields that are rejected, and those old that are not. In the
long run, nearly all output fields would be marked as NLA_REJECT,
okay.
Then I ask my first question again: why reject these? They are not
hurting anything, are they? It's different from your example I think.
In there, the extra information which was ignored leads to a
different behavior.
Maybe it would be better to have NLA_IGNORE instead? </idea>
Marcelo
On Thu, Sep 13, 2018 at 10:46:02AM +0200, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> In some situations some netlink attributes may be used for output
> only (kernel->userspace) or may be reserved for future use. It's
> then helpful to be able to prevent userspace from using them in
> messages sent to the kernel, since they'd otherwise be ignored and
> any future will become impossible if this happens.
>
> Add NLA_REJECT to the policy which does nothing but reject (with
> EINVAL) validation of any messages containing this attribute.
> Allow for returning a specific extended ACK error message in the
> validation_data pointer.
>
> While at it fix the indentation of NLA_BITFIELD32 and describe the
> validation_data pointer for it.
>
> The specific case I have in mind now is a shared nested attribute
> containing request/response data, and it would be pointless and
> potentially confusing to have userspace include response data in
> the messages that actually contain a request.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> ...
> @@ -251,10 +256,13 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
>
> if (type > 0 && type <= maxtype) {
> if (policy) {
> - err = validate_nla(nla, maxtype, policy);
> + err = validate_nla(nla, maxtype, policy,
> + extack);
> if (err < 0) {
> - NL_SET_ERR_MSG_ATTR(extack, nla,
> - "Attribute failed policy validation");
> + NL_SET_BAD_ATTR(extack, nla);
> + if (extack && !extack->_msg)
> + NL_SET_ERR_MSG(extack,
> + "Attribute failed policy validation");
> goto errout;
> }
> }
> --
Technically, this would change the outcome when nla_parse() is called
with extack->_msg already set nad validate_nla() fails on something else
than NLA_REJECT; it will preserve the previous message in such case.
But I don't think this is a serious problem.
Reviewed-by: Michal Kubecek <[email protected]>
On Thu, Sep 13, 2018 at 02:05:54PM +0200, Michal Kubecek wrote:
> On Thu, Sep 13, 2018 at 01:25:15PM +0200, Johannes Berg wrote:
> > On Thu, 2018-09-13 at 12:49 +0200, Michal Kubecek wrote:
> >
> > > > if (type > 0 && type <= maxtype) {
> > > > if (policy) {
> > > > - err = validate_nla(nla, maxtype, policy);
> > > > + err = validate_nla(nla, maxtype, policy,
> > > > + extack);
> > > > if (err < 0) {
> > > > - NL_SET_ERR_MSG_ATTR(extack, nla,
> > > > - "Attribute failed policy validation");
> > > > + NL_SET_BAD_ATTR(extack, nla);
> > > > + if (extack && !extack->_msg)
> > > > + NL_SET_ERR_MSG(extack,
> > > > + "Attribute failed policy validation");
> > > > goto errout;
> > > > }
> > > > }
> > > > --
> > >
> > > Technically, this would change the outcome when nla_parse() is called
> > > with extack->_msg already set nad validate_nla() fails on something else
> > > than NLA_REJECT; it will preserve the previous message in such case.
> > > But I don't think this is a serious problem.
> >
> > Yes, that's true. I looked at quite a few of the setters just now (there
> > are ~500 already, wow!), and they all set & return, so this shouldn't be
> > an issue.
>
> In ethtool (work in progress) I sometimes use extack message for
> non-fatal warnings but AFAICS never before parsing the userspace
> request (actually always shortly before returning). So I don't have
> a problem with it.
Considering we can only report 1 message, it should be okay to drop
the previous message in favor of the new one, which is either a
critical one or just another non-fatal one.
Marcelo
On Thu, Sep 13, 2018 at 04:41:16PM -0300, Marcelo Ricardo Leitner wrote:
> On Thu, Sep 13, 2018 at 02:16:06PM +0200, Johannes Berg wrote:
> > On Thu, 2018-09-13 at 14:12 +0200, Michal Kubecek wrote:
> > > On Thu, Sep 13, 2018 at 02:02:53PM +0200, Johannes Berg wrote:
> > > > On Thu, 2018-09-13 at 13:58 +0200, Michal Kubecek wrote:
> > > >
> > > > > The code looks correct to me but I have some doubts. Having a special
> > > > > policy for MAC addresses may lead to adding one for IPv4 address (maybe
> > > > > not, we can use NLA_U32 for them), IPv6 addresses and other data types
> > > > > with fixed length. Wouldn't it be more helpful to add a variant of
> > > > > NLA_BINARY (NLA_BINARY_EXACT?) which would fail/warn if attribute length
> > > > > isn't equal to .len?
> > > >
> > > > Yeah, I guess we could do that, and then
> > > >
> > > > #define NLA_ETH_ADDR .len = ETH_ALEN, .type = NLA_BINARY_EXACT
> > > > #define NLA_IP6_ADDR .len = 16, .type = NLA_BINARY_EXACT
> > > >
> > > > or so?
> > >
> > > Maybe rather
> > >
> > > #define NLA_ETH_ADDR NLA_BINARY_EXACT, .len = ETH_ALEN
> > > #define NLA_IP6_ADDR NLA_BINARY_EXACT, .len = sizeof(struct in6_addr)
> > >
> > > so that one could write
> > >
> > > { .type = NLA_ETH_ADDR }
> >
> > Yeah, that's possible. I considered it for a second, but it was slightly
> > too magical for my taste :-)
> >
> > Better pick a different "namespace", perhaps NLA_POLICY_ETH_ADDR or so?
>
> What about
> #define NLA_FIELD_ETH_ADDR { .type = NLA_BINARY_EXACT, .len = ETH_ALEN }
>
> Or even
> #define NLA_FIELD_BINARY_EXACT(_len) { .type = NLA_BINARY_EXACT, .len = (_len) }
> #define NLA_FIELD_ETH_ADDR NLA_FIELD_BINARY_EXACT(ETH_ALEN)
>
> So that one would just:
> [MYADDR] = NLA_FIELD_ETH_ADDR,
Yes, that's how I understoold Johannes' proposal above.
Michal Kubecek
On Thu, Sep 13, 2018 at 02:46:17PM +0200, Johannes Berg wrote:
> On Thu, 2018-09-13 at 14:24 +0200, Michal Kubecek wrote:
> > On Thu, Sep 13, 2018 at 02:16:06PM +0200, Johannes Berg wrote:
> > > On Thu, 2018-09-13 at 14:12 +0200, Michal Kubecek wrote:
> > > > Maybe rather
> > > >
> > > > #define NLA_ETH_ADDR NLA_BINARY_EXACT, .len = ETH_ALEN
> > > > #define NLA_IP6_ADDR NLA_BINARY_EXACT, .len = sizeof(struct in6_addr)
> > > >
> > > > so that one could write
> > > >
> > > > { .type = NLA_ETH_ADDR }
> > >
> > > Yeah, that's possible. I considered it for a second, but it was slightly
> > > too magical for my taste :-)
> > >
> > > Better pick a different "namespace", perhaps NLA_POLICY_ETH_ADDR or so?
> >
> > Right, that sounds better. I'm afraid anything too tricky would
> > inevitably trick people into using it in an unexpected way and ending up
> > with very confusing error messages.
>
> Right.
>
> Then again though, we already have NLA_MSECS which is basically
> equivalent to NLA_U64 afaict, so why not have more types?
>
> It doesn't really cost us that much, just a few bytes in the validation?
And one more branch in the switch in validate_nla().
I'm not really opposed to having special policy types for MAC/IPv4/IPv6
address, these types are quite common, at least in networking code which
is where netlink is used most oftena. I rather felt that technically the
only difference between MAC and IPv6 address is the size and as we have
.len already, it would be more useful to have generic policy allowing to
also handle other fixes size data types.
On the other hand, if there was a trend of adding special policies for
more "endemic" data types, it might be more appropriate to introduce
a generic policy where validation_data would be a "validator function"
doing specific checks (probably using a union to allow type check of the
callback). But that's not happening (yet).
> Also, with .type = NLA_ETH_ADDR_COMPAT we could get a warning, which is
> not true for just checking .len.
We could have three flavors of NLA_BINARY.
Michal Kubecek
On Thu, Sep 13, 2018 at 04:30:04PM -0300, Marcelo Ricardo Leitner wrote:
> On Thu, Sep 13, 2018 at 10:46:02AM +0200, Johannes Berg wrote:
> > From: Johannes Berg <[email protected]>
> >
> > In some situations some netlink attributes may be used for output
> > only (kernel->userspace) or may be reserved for future use. It's
> > then helpful to be able to prevent userspace from using them in
> > messages sent to the kernel, since they'd otherwise be ignored and
> > any future will become impossible if this happens.
>
> I don't follow, what's the issue with simply ignoring such attributes?
A bit artificial example but I can't come with somethin better at the
moment: let's say newer kernel and iproute2 allow something like
ip link del <condition1> <condition2>
and you run new ip with older kernel which only supports <condition1>.
You don't really want kernel to silently ignore the second condition.
Or e.g. when adding a netfilter rule, you wouldn't want kernel to ignore
parts of the rule it does not understand.
I must admit I'm not sure if there is really need for having reserved
attributes with netlink. Maybe e.g. when we want to share part of
(numeric) attribute types between different message types. Anyway, we
have the same problem with attributes higher than maximum; NLA_REJECT
wouldn't help with this but the discussion also touched the topic.
> > Add NLA_REJECT to the policy which does nothing but reject (with
> > EINVAL) validation of any messages containing this attribute.
> > Allow for returning a specific extended ACK error message in the
> > validation_data pointer.
>
> This has potential to create confusion because we can't use it on
> {output,reserved} attributes that are already there (as we must always
> ignore them now) and we will end up with a mix of it.
We can return -EINVAL even now, we just need to add a check after
nla_parse() wrapper returns, e.g. here: (lines 314-320)
https://github.com/mkubecek/ethnl/blob/master/kernel/0019-ethtool-implement-SET_PARAMS-message.patch#L314
It would be easier and IMHO cleaner if I could simply list these "read
only attributes" with NLA_REJECT policy for "set" request.
Michal Kubecek
From: Johannes Berg <[email protected]>
Date: Thu, 13 Sep 2018 10:46:02 +0200
> + NL_SET_BAD_ATTR(extack, nla);
> + if (extack && !extack->_msg)
> + NL_SET_ERR_MSG(extack,
> + "Attribute failed policy validation");
Given the lively discussion that resulted from this conditional I am
pretty sure we want to override existing messages.
If we have an existing message, and we continued to process and
parse anyways, then the existing message was informational or
a warning.
The message should be overridden when the action will be to fail, as
it will be here when we return -EINVAL.
On Thu, 2018-09-13 at 13:58 +0200, Michal Kubecek wrote:
> The code looks correct to me but I have some doubts. Having a special
> policy for MAC addresses may lead to adding one for IPv4 address (maybe
> not, we can use NLA_U32 for them), IPv6 addresses and other data types
> with fixed length. Wouldn't it be more helpful to add a variant of
> NLA_BINARY (NLA_BINARY_EXACT?) which would fail/warn if attribute length
> isn't equal to .len?
Yeah, I guess we could do that, and then
#define NLA_ETH_ADDR .len = ETH_ALEN, .type = NLA_BINARY_EXACT
#define NLA_IP6_ADDR .len = 16, .type = NLA_BINARY_EXACT
or so?
johannes
On Tue, 2018-09-18 at 08:34 -0400, Jamal Hadi Salim wrote:
> > > Maybe it would be better to have NLA_IGNORE instead? </idea>
> >
> > I don't think so, it doesn't give any feedback to the application author
> > that they're doing something wrong.
> >
>
> Maybe time to introduce kernel side access-control flags?
> Read/Write permissions for example. Attrs marked as read only
> (in the kernel) cannot be written to.
I dunno, that might work for ethtool, but I want to use it for something
that's not even an attribute you could think about writing to, but the
result of some operation you started.
What would the practical difference be though? Hopefully you wouldn't
have write-only attributes, and then NLA_REJECT is basically equivalent?
johannes
On 2018-09-18 8:39 a.m., Johannes Berg wrote:
> On Tue, 2018-09-18 at 08:34 -0400, Jamal Hadi Salim wrote:
>
>> Maybe time to introduce kernel side access-control flags?
>> Read/Write permissions for example. Attrs marked as read only
>> (in the kernel) cannot be written to.
>
> I dunno, that might work for ethtool, but I want to use it for something
> that's not even an attribute you could think about writing to, but the
> result of some operation you started.
>
Execute permission kind of thing? i.e if i understood you correctly
if acl is "rwx" then attribute can only be written to (or read from) if
the "thing executing" is complete
> What would the practical difference be though? Hopefully you wouldn't
> have write-only attributes, and then NLA_REJECT is basically equivalent?
>
If ACL says "-w-" then reading should get explicit permission denied
code possibly with an extack which is more descriptive that reading
is not allowed.
cheers,
jamal
On 2018-09-17 5:38 a.m., Johannes Berg wrote:
> On Thu, 2018-09-13 at 18:58 -0300, Marcelo Ricardo Leitner wrote:
>
[..]
>
> So in one case I was thinking of, there are some fields that simply
> cannot be used for input, they're only used for output. > But it may not
> always be obvious to somebody using the API. Thus, I think it makes
> sense to instruct the kernel to reject that, so that whoever gets
> confused has immediate feedback that their usage is wrong. If we ignore
> that, they may not realize their error immediately.
>
> I think the ethtool case is similar: you can read and write some fields,
> and only read others - but if you try to write the read-only fields
> would you prefer to be told "sorry, this is not possible" vs. it being
> silently ignored? I'd definitely prefer the former.
>
>> Maybe it would be better to have NLA_IGNORE instead? </idea>
>
> I don't think so, it doesn't give any feedback to the application author
> that they're doing something wrong.
>
Maybe time to introduce kernel side access-control flags?
Read/Write permissions for example. Attrs marked as read only
(in the kernel) cannot be written to.
cheers,
jamal