All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-02 20:23 ` Johannes Berg
  0 siblings, 0 replies; 17+ messages in thread
From: Johannes Berg @ 2017-12-02 20:23 UTC (permalink / raw)
  To: linux-wireless, netdev; +Cc: j, David Ahern, Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

This netlink type is used only for backwards compatibility
with broken userspace that used the wrong size for a given
u8 attribute, which is now rejected. It would've been wrong
before already, since on big endian the wrong value (always
zero) would be used by the kernel, but we can't break the
existing deployed userspace - hostapd for example now fails
to initialize entirely.

We could try to fix up the big endian problem here, but we
don't know *how* userspace misbehaved - if using nla_put_u32
then we could, but we also found a debug tool (which we'll
ignore for the purposes of this regression) that was putting
the padding into the length.

Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h | 1 +
 lib/nlattr.c          | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0c154f98e987..448a9b86c959 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -180,6 +180,7 @@ enum {
 	NLA_S32,
 	NLA_S64,
 	NLA_BITFIELD32,
+	NLA_U8_BUGGY, /* don't use this - only for bug-ward compatibility */
 	__NLA_TYPE_MAX,
 };
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..2b89d25d4745 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -28,6 +28,7 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
 };
 
 static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+	[NLA_U8_BUGGY]	= sizeof(u8),
 	[NLA_MSECS]	= sizeof(u64),
 	[NLA_NESTED]	= NLA_HDRLEN,
 };
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-02 20:23 ` Johannes Berg
  0 siblings, 0 replies; 17+ messages in thread
From: Johannes Berg @ 2017-12-02 20:23 UTC (permalink / raw)
  To: linux-wireless-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: j, David Ahern, Johannes Berg

From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This netlink type is used only for backwards compatibility
with broken userspace that used the wrong size for a given
u8 attribute, which is now rejected. It would've been wrong
before already, since on big endian the wrong value (always
zero) would be used by the kernel, but we can't break the
existing deployed userspace - hostapd for example now fails
to initialize entirely.

We could try to fix up the big endian problem here, but we
don't know *how* userspace misbehaved - if using nla_put_u32
then we could, but we also found a debug tool (which we'll
ignore for the purposes of this regression) that was putting
the padding into the length.

Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 include/net/netlink.h | 1 +
 lib/nlattr.c          | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0c154f98e987..448a9b86c959 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -180,6 +180,7 @@ enum {
 	NLA_S32,
 	NLA_S64,
 	NLA_BITFIELD32,
+	NLA_U8_BUGGY, /* don't use this - only for bug-ward compatibility */
 	__NLA_TYPE_MAX,
 };
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..2b89d25d4745 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -28,6 +28,7 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
 };
 
 static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+	[NLA_U8_BUGGY]	= sizeof(u8),
 	[NLA_MSECS]	= sizeof(u64),
 	[NLA_NESTED]	= NLA_HDRLEN,
 };
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net 2/2] nl80211: use NLA_U8_BUGGY for two attributes
  2017-12-02 20:23 ` Johannes Berg
  (?)
@ 2017-12-02 20:23 ` Johannes Berg
  -1 siblings, 0 replies; 17+ messages in thread
From: Johannes Berg @ 2017-12-02 20:23 UTC (permalink / raw)
  To: linux-wireless, netdev; +Cc: j, David Ahern, Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

We discovered that these are set incorrectly by the
corresponding userspace code, so keep compatible with
their bugs even if they'd always set the value to 0.

Reported-by: Jouni Malinen <j@w1.fi>
Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/nl80211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b1ac23ca20c8..751b4efbf09a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -384,7 +384,7 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_TSID] = { .type = NLA_U8 },
 	[NL80211_ATTR_USER_PRIO] = { .type = NLA_U8 },
 	[NL80211_ATTR_ADMITTED_TIME] = { .type = NLA_U16 },
-	[NL80211_ATTR_SMPS_MODE] = { .type = NLA_U8 },
+	[NL80211_ATTR_SMPS_MODE] = { .type = NLA_U8_BUGGY },
 	[NL80211_ATTR_MAC_MASK] = { .len = ETH_ALEN },
 	[NL80211_ATTR_WIPHY_SELF_MANAGED_REG] = { .type = NLA_FLAG },
 	[NL80211_ATTR_NETNS_FD] = { .type = NLA_U32 },
@@ -5830,7 +5830,7 @@ static const struct nla_policy nl80211_meshconf_params_policy[NL80211_MESHCONF_A
 	[NL80211_MESHCONF_MAX_RETRIES] = { .type = NLA_U8 },
 	[NL80211_MESHCONF_TTL] = { .type = NLA_U8 },
 	[NL80211_MESHCONF_ELEMENT_TTL] = { .type = NLA_U8 },
-	[NL80211_MESHCONF_AUTO_OPEN_PLINKS] = { .type = NLA_U8 },
+	[NL80211_MESHCONF_AUTO_OPEN_PLINKS] = { .type = NLA_U8_BUGGY },
 	[NL80211_MESHCONF_SYNC_OFFSET_MAX_NEIGHBOR] = { .type = NLA_U32 },
 	[NL80211_MESHCONF_HWMP_MAX_PREQ_RETRIES] = { .type = NLA_U8 },
 	[NL80211_MESHCONF_PATH_REFRESH_TIME] = { .type = NLA_U32 },
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-02 22:48   ` David Ahern
  0 siblings, 0 replies; 17+ messages in thread
From: David Ahern @ 2017-12-02 22:48 UTC (permalink / raw)
  To: Johannes Berg, linux-wireless, netdev; +Cc: j, Johannes Berg

On 12/2/17 1:23 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> This netlink type is used only for backwards compatibility
> with broken userspace that used the wrong size for a given
> u8 attribute, which is now rejected. It would've been wrong
> before already, since on big endian the wrong value (always
> zero) would be used by the kernel, but we can't break the
> existing deployed userspace - hostapd for example now fails
> to initialize entirely.
> 
> We could try to fix up the big endian problem here, but we
> don't know *how* userspace misbehaved - if using nla_put_u32
> then we could, but we also found a debug tool (which we'll
> ignore for the purposes of this regression) that was putting
> the padding into the length.
> 
> Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Hi Johannes:

I have been really busy the past 2 weeks, so have not gotten around to
dealing with this. I was planning to partially revert 28033ae4e0f5 --
change it from failure to log an error message so buggy commands can be
fixed.

David

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-02 22:48   ` David Ahern
  0 siblings, 0 replies; 17+ messages in thread
From: David Ahern @ 2017-12-02 22:48 UTC (permalink / raw)
  To: Johannes Berg, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: j, Johannes Berg

On 12/2/17 1:23 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This netlink type is used only for backwards compatibility
> with broken userspace that used the wrong size for a given
> u8 attribute, which is now rejected. It would've been wrong
> before already, since on big endian the wrong value (always
> zero) would be used by the kernel, but we can't break the
> existing deployed userspace - hostapd for example now fails
> to initialize entirely.
> 
> We could try to fix up the big endian problem here, but we
> don't know *how* userspace misbehaved - if using nla_put_u32
> then we could, but we also found a debug tool (which we'll
> ignore for the purposes of this regression) that was putting
> the padding into the length.
> 
> Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
> Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Hi Johannes:

I have been really busy the past 2 weeks, so have not gotten around to
dealing with this. I was planning to partially revert 28033ae4e0f5 --
change it from failure to log an error message so buggy commands can be
fixed.

David

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
  2017-12-02 20:23 ` Johannes Berg
                   ` (2 preceding siblings ...)
  (?)
@ 2017-12-05 16:31 ` David Miller
  2017-12-05 16:34     ` Johannes Berg
  -1 siblings, 1 reply; 17+ messages in thread
From: David Miller @ 2017-12-05 16:31 UTC (permalink / raw)
  To: johannes; +Cc: linux-wireless, netdev, j, dsahern, johannes.berg

From: Johannes Berg <johannes@sipsolutions.net>
Date: Sat,  2 Dec 2017 21:23:31 +0100

> From: Johannes Berg <johannes.berg@intel.com>
> 
> This netlink type is used only for backwards compatibility
> with broken userspace that used the wrong size for a given
> u8 attribute, which is now rejected. It would've been wrong
> before already, since on big endian the wrong value (always
> zero) would be used by the kernel, but we can't break the
> existing deployed userspace - hostapd for example now fails
> to initialize entirely.
> 
> We could try to fix up the big endian problem here, but we
> don't know *how* userspace misbehaved - if using nla_put_u32
> then we could, but we also found a debug tool (which we'll
> ignore for the purposes of this regression) that was putting
> the padding into the length.
> 
> Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

We're stuck with this thing forever... I'd like to consider other
options.

I've seen this problem at least one time before, therefore I
suggest when we see a U8 attribute with a U32's length:

1) We access it as a u32, this takes care of all endianness
   issues.

2) We emit a warning so that the app gets fixes.

Thanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-05 16:34     ` Johannes Berg
  0 siblings, 0 replies; 17+ messages in thread
From: Johannes Berg @ 2017-12-05 16:34 UTC (permalink / raw)
  To: David Miller; +Cc: linux-wireless, netdev, j, dsahern

On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
> 
> > We could try to fix up the big endian problem here, but we
> > don't know *how* userspace misbehaved - if using nla_put_u32
> > then we could, but we also found a debug tool (which we'll
> > ignore for the purposes of this regression) that was putting
> > the padding into the length.

> We're stuck with this thing forever... I'd like to consider other
> options.
> 
> I've seen this problem at least one time before, therefore I
> suggest when we see a U8 attribute with a U32's length:
> 
> 1) We access it as a u32, this takes care of all endianness
>    issues.

Possible, but as I said above, I've seen at least one tool (a debug
only script) now that will actually emit a U8 followed by 3 bytes of
padding to make it netlink-aligned, but set the length to 4. That would
be broken by making this change.

I'm not saying this is bad - but there are different levels of
compatibility and I'd probably go for "bug compatibility" here rather
than "fix-it-up compatibility".

Your call, ultimately - I've already fixed the tool I had found :-)

> 2) We emit a warning so that the app gets fixes.

For sure.

johannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-05 16:34     ` Johannes Berg
  0 siblings, 0 replies; 17+ messages in thread
From: Johannes Berg @ 2017-12-05 16:34 UTC (permalink / raw)
  To: David Miller
  Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA, j, dsahern-Re5JQEeQqe8AvxtiuMwx3w

On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
> 
> > We could try to fix up the big endian problem here, but we
> > don't know *how* userspace misbehaved - if using nla_put_u32
> > then we could, but we also found a debug tool (which we'll
> > ignore for the purposes of this regression) that was putting
> > the padding into the length.

> We're stuck with this thing forever... I'd like to consider other
> options.
> 
> I've seen this problem at least one time before, therefore I
> suggest when we see a U8 attribute with a U32's length:
> 
> 1) We access it as a u32, this takes care of all endianness
>    issues.

Possible, but as I said above, I've seen at least one tool (a debug
only script) now that will actually emit a U8 followed by 3 bytes of
padding to make it netlink-aligned, but set the length to 4. That would
be broken by making this change.

I'm not saying this is bad - but there are different levels of
compatibility and I'd probably go for "bug compatibility" here rather
than "fix-it-up compatibility".

Your call, ultimately - I've already fixed the tool I had found :-)

> 2) We emit a warning so that the app gets fixes.

For sure.

johannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
  2017-12-05 16:34     ` Johannes Berg
  (?)
@ 2017-12-05 16:41     ` David Miller
  2017-12-05 17:30       ` Johannes Berg
  -1 siblings, 1 reply; 17+ messages in thread
From: David Miller @ 2017-12-05 16:41 UTC (permalink / raw)
  To: johannes; +Cc: linux-wireless, netdev, j, dsahern

From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 05 Dec 2017 17:34:21 +0100

> On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
>> 
>> > We could try to fix up the big endian problem here, but we
>> > don't know *how* userspace misbehaved - if using nla_put_u32
>> > then we could, but we also found a debug tool (which we'll
>> > ignore for the purposes of this regression) that was putting
>> > the padding into the length.
> 
>> We're stuck with this thing forever... I'd like to consider other
>> options.
>> 
>> I've seen this problem at least one time before, therefore I
>> suggest when we see a U8 attribute with a U32's length:
>> 
>> 1) We access it as a u32, this takes care of all endianness
>>    issues.
> 
> Possible, but as I said above, I've seen at least one tool (a debug
> only script) now that will actually emit a U8 followed by 3 bytes of
> padding to make it netlink-aligned, but set the length to 4. That would
> be broken by making this change.

There is no reasonable interpretation for what that application is
doing, so I think we can safely call that case as buggy.

We are only trying to handle the situation where a U8 attribute
is presented as a bonafide U32 or a correct U8.

Does this make sense?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
  2017-12-05 16:34     ` Johannes Berg
  (?)
  (?)
@ 2017-12-05 16:41     ` David Ahern
  2017-12-05 16:51         ` David Miller
  -1 siblings, 1 reply; 17+ messages in thread
From: David Ahern @ 2017-12-05 16:41 UTC (permalink / raw)
  To: Johannes Berg, David Miller; +Cc: linux-wireless, netdev, j

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

On 12/5/17 9:34 AM, Johannes Berg wrote:
> On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
>>
>>> We could try to fix up the big endian problem here, but we
>>> don't know *how* userspace misbehaved - if using nla_put_u32
>>> then we could, but we also found a debug tool (which we'll
>>> ignore for the purposes of this regression) that was putting
>>> the padding into the length.
> 
>> We're stuck with this thing forever... I'd like to consider other
>> options.
>>
>> I've seen this problem at least one time before, therefore I
>> suggest when we see a U8 attribute with a U32's length:
>>
>> 1) We access it as a u32, this takes care of all endianness
>>    issues.
> 
> Possible, but as I said above, I've seen at least one tool (a debug
> only script) now that will actually emit a U8 followed by 3 bytes of
> padding to make it netlink-aligned, but set the length to 4. That would
> be broken by making this change.
> 
> I'm not saying this is bad - but there are different levels of
> compatibility and I'd probably go for "bug compatibility" here rather
> than "fix-it-up compatibility".
> 
> Your call, ultimately - I've already fixed the tool I had found :-)
> 
>> 2) We emit a warning so that the app gets fixes.
> 

The attached is my proposal. Basically, allow it the length mismatch but
print a warning. This restores previous behavior and tells users of bad
commands.

[-- Attachment #2: 0001-netlink-Relax-attr-validation-for-fixed-length-types.patch --]
[-- Type: text/plain, Size: 2222 bytes --]

From 29a504c8de553c17d4aafd5a2cb9384a28672fe4 Mon Sep 17 00:00:00 2001
From: David Ahern <dsahern@gmail.com>
Date: Sun, 3 Dec 2017 07:22:20 -0800
Subject: [PATCH] netlink: Relax attr validation for fixed length types

Commit 28033ae4e0f5 ("net: netlink: Update attr validation to require
exact length for some types") requires attributes using types NLA_U* and
NLA_S* to have an exact length. This change is exposing bugs in various
userspace commands that are sending attributes with an invalid length
(e.g., attribute has type NLA_U8 and userspace sends NLA_U32). While
the commands are clearly broken and need to be fixed, users are arguing
that the sudden change in enforcement is breaking older commands on
newer kernels for use cases that otherwise "worked".

Relax the validation to print a warning mesage similar to what is done
for messages containing extra bytes after parsing.

Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
Signed-off-by: David Ahern <dsahern@gmail.com>
---
 lib/nlattr.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..6122662906c8 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -28,8 +28,16 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
 };
 
 static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+	[NLA_U8]	= sizeof(u8),
+	[NLA_U16]	= sizeof(u16),
+	[NLA_U32]	= sizeof(u32),
+	[NLA_U64]	= sizeof(u64),
 	[NLA_MSECS]	= sizeof(u64),
 	[NLA_NESTED]	= NLA_HDRLEN,
+	[NLA_S8]	= sizeof(s8),
+	[NLA_S16]	= sizeof(s16),
+	[NLA_S32]	= sizeof(s32),
+	[NLA_S64]	= sizeof(s64),
 };
 
 static int validate_nla_bitfield32(const struct nlattr *nla,
@@ -70,10 +78,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 	BUG_ON(pt->type > NLA_TYPE_MAX);
 
 	/* for data types NLA_U* and NLA_S* require exact length */
-	if (nla_attr_len[pt->type]) {
-		if (attrlen != nla_attr_len[pt->type])
-			return -ERANGE;
-		return 0;
+	if (nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) {
+		pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
+				    current->comm, type);
 	}
 
 	switch (pt->type) {
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-05 16:51         ` David Miller
  0 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2017-12-05 16:51 UTC (permalink / raw)
  To: dsahern; +Cc: johannes, linux-wireless, netdev, j

From: David Ahern <dsahern@gmail.com>
Date: Tue, 5 Dec 2017 09:41:21 -0700

> On 12/5/17 9:34 AM, Johannes Berg wrote:
>> On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
>>>
>>>> We could try to fix up the big endian problem here, but we
>>>> don't know *how* userspace misbehaved - if using nla_put_u32
>>>> then we could, but we also found a debug tool (which we'll
>>>> ignore for the purposes of this regression) that was putting
>>>> the padding into the length.
>> 
>>> We're stuck with this thing forever... I'd like to consider other
>>> options.
>>>
>>> I've seen this problem at least one time before, therefore I
>>> suggest when we see a U8 attribute with a U32's length:
>>>
>>> 1) We access it as a u32, this takes care of all endianness
>>>    issues.
>> 
>> Possible, but as I said above, I've seen at least one tool (a debug
>> only script) now that will actually emit a U8 followed by 3 bytes of
>> padding to make it netlink-aligned, but set the length to 4. That would
>> be broken by making this change.
>> 
>> I'm not saying this is bad - but there are different levels of
>> compatibility and I'd probably go for "bug compatibility" here rather
>> than "fix-it-up compatibility".
>> 
>> Your call, ultimately - I've already fixed the tool I had found :-)
>> 
>>> 2) We emit a warning so that the app gets fixes.
>> 
> 
> The attached is my proposal. Basically, allow it the length mismatch but
> print a warning. This restores previous behavior and tells users of bad
> commands.

Where is the "access the U8 attribute as a U32 if length is 4" part
of my #1 above?

That's essential to handle this properly on all endianness.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-05 16:51         ` David Miller
  0 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2017-12-05 16:51 UTC (permalink / raw)
  To: dsahern-Re5JQEeQqe8AvxtiuMwx3w
  Cc: johannes-cdvu00un1VgdHxzADdlk8Q,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA, j

From: David Ahern <dsahern-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Tue, 5 Dec 2017 09:41:21 -0700

> On 12/5/17 9:34 AM, Johannes Berg wrote:
>> On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
>>>
>>>> We could try to fix up the big endian problem here, but we
>>>> don't know *how* userspace misbehaved - if using nla_put_u32
>>>> then we could, but we also found a debug tool (which we'll
>>>> ignore for the purposes of this regression) that was putting
>>>> the padding into the length.
>> 
>>> We're stuck with this thing forever... I'd like to consider other
>>> options.
>>>
>>> I've seen this problem at least one time before, therefore I
>>> suggest when we see a U8 attribute with a U32's length:
>>>
>>> 1) We access it as a u32, this takes care of all endianness
>>>    issues.
>> 
>> Possible, but as I said above, I've seen at least one tool (a debug
>> only script) now that will actually emit a U8 followed by 3 bytes of
>> padding to make it netlink-aligned, but set the length to 4. That would
>> be broken by making this change.
>> 
>> I'm not saying this is bad - but there are different levels of
>> compatibility and I'd probably go for "bug compatibility" here rather
>> than "fix-it-up compatibility".
>> 
>> Your call, ultimately - I've already fixed the tool I had found :-)
>> 
>>> 2) We emit a warning so that the app gets fixes.
>> 
> 
> The attached is my proposal. Basically, allow it the length mismatch but
> print a warning. This restores previous behavior and tells users of bad
> commands.

Where is the "access the U8 attribute as a U32 if length is 4" part
of my #1 above?

That's essential to handle this properly on all endianness.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
  2017-12-05 16:41     ` David Miller
@ 2017-12-05 17:30       ` Johannes Berg
  2017-12-05 17:40         ` David Miller
  0 siblings, 1 reply; 17+ messages in thread
From: Johannes Berg @ 2017-12-05 17:30 UTC (permalink / raw)
  To: David Miller; +Cc: linux-wireless, netdev, j, dsahern

On Tue, 2017-12-05 at 11:41 -0500, David Miller wrote:
> 
> There is no reasonable interpretation for what that application is
> doing, so I think we can safely call that case as buggy.
> 
> We are only trying to handle the situation where a U8 attribute
> is presented as a bonafide U32 or a correct U8.
> 
> Does this make sense?

Well the application is buggy, but we don't really know in what way?
Perhaps somebody even did the equivalent of
    nla_put_u32(ATTR, cpu_to_le32(x))
when they noticed it was broken on BE, and end up with a similar case
as I had above.

I don't think there's a good solution to this, applications must be
fixed anyhow. I'm just saying that I'd save the extra code and stay
compatible with applications as written today, even if they're now
broken on BE - and rely on the warning to fix it. Trying to fix it up
seems to have the potential to just break something else.

johannes

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
  2017-12-05 17:30       ` Johannes Berg
@ 2017-12-05 17:40         ` David Miller
  2017-12-05 18:15             ` David Ahern
  0 siblings, 1 reply; 17+ messages in thread
From: David Miller @ 2017-12-05 17:40 UTC (permalink / raw)
  To: johannes; +Cc: linux-wireless, netdev, j, dsahern

From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 05 Dec 2017 18:30:10 +0100

> On Tue, 2017-12-05 at 11:41 -0500, David Miller wrote:
>> 
>> There is no reasonable interpretation for what that application is
>> doing, so I think we can safely call that case as buggy.
>> 
>> We are only trying to handle the situation where a U8 attribute
>> is presented as a bonafide U32 or a correct U8.
>> 
>> Does this make sense?
> 
> Well the application is buggy, but we don't really know in what way?
> Perhaps somebody even did the equivalent of
>     nla_put_u32(ATTR, cpu_to_le32(x))
> when they noticed it was broken on BE, and end up with a similar case
> as I had above.
> 
> I don't think there's a good solution to this, applications must be
> fixed anyhow. I'm just saying that I'd save the extra code and stay
> compatible with applications as written today, even if they're now
> broken on BE - and rely on the warning to fix it. Trying to fix it up
> seems to have the potential to just break something else.

You might be right.

Ok let's just go with the warning + existing behavior for now.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-05 18:15             ` David Ahern
  0 siblings, 0 replies; 17+ messages in thread
From: David Ahern @ 2017-12-05 18:15 UTC (permalink / raw)
  To: David Miller, johannes; +Cc: linux-wireless, netdev, j

On 12/5/17 10:40 AM, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Tue, 05 Dec 2017 18:30:10 +0100
> 
>> On Tue, 2017-12-05 at 11:41 -0500, David Miller wrote:
>>>
>>> There is no reasonable interpretation for what that application is
>>> doing, so I think we can safely call that case as buggy.
>>>
>>> We are only trying to handle the situation where a U8 attribute
>>> is presented as a bonafide U32 or a correct U8.
>>>
>>> Does this make sense?
>>
>> Well the application is buggy, but we don't really know in what way?
>> Perhaps somebody even did the equivalent of
>>     nla_put_u32(ATTR, cpu_to_le32(x))
>> when they noticed it was broken on BE, and end up with a similar case
>> as I had above.
>>
>> I don't think there's a good solution to this, applications must be
>> fixed anyhow. I'm just saying that I'd save the extra code and stay
>> compatible with applications as written today, even if they're now
>> broken on BE - and rely on the warning to fix it. Trying to fix it up
>> seems to have the potential to just break something else.

+1

> 
> You might be right.
> 
> Ok let's just go with the warning + existing behavior for now.

Is the patch I sent as an attachment good or should I re-send
standalone? (don't see it in patchwork)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
@ 2017-12-05 18:15             ` David Ahern
  0 siblings, 0 replies; 17+ messages in thread
From: David Ahern @ 2017-12-05 18:15 UTC (permalink / raw)
  To: David Miller, johannes-cdvu00un1VgdHxzADdlk8Q
  Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, j

On 12/5/17 10:40 AM, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Tue, 05 Dec 2017 18:30:10 +0100
> 
>> On Tue, 2017-12-05 at 11:41 -0500, David Miller wrote:
>>>
>>> There is no reasonable interpretation for what that application is
>>> doing, so I think we can safely call that case as buggy.
>>>
>>> We are only trying to handle the situation where a U8 attribute
>>> is presented as a bonafide U32 or a correct U8.
>>>
>>> Does this make sense?
>>
>> Well the application is buggy, but we don't really know in what way?
>> Perhaps somebody even did the equivalent of
>>     nla_put_u32(ATTR, cpu_to_le32(x))
>> when they noticed it was broken on BE, and end up with a similar case
>> as I had above.
>>
>> I don't think there's a good solution to this, applications must be
>> fixed anyhow. I'm just saying that I'd save the extra code and stay
>> compatible with applications as written today, even if they're now
>> broken on BE - and rely on the warning to fix it. Trying to fix it up
>> seems to have the potential to just break something else.

+1

> 
> You might be right.
> 
> Ok let's just go with the warning + existing behavior for now.

Is the patch I sent as an attachment good or should I re-send
standalone? (don't see it in patchwork)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
  2017-12-05 18:15             ` David Ahern
  (?)
@ 2017-12-05 19:08             ` David Miller
  -1 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2017-12-05 19:08 UTC (permalink / raw)
  To: dsahern; +Cc: johannes, linux-wireless, netdev, j

From: David Ahern <dsahern@gmail.com>
Date: Tue, 5 Dec 2017 11:15:29 -0700

> Is the patch I sent as an attachment good or should I re-send
> standalone? (don't see it in patchwork)

Patchwork has been wonky laterly, please resubmit as a fresh
email for rewiew.

Thanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-12-05 19:08 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-02 20:23 [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type Johannes Berg
2017-12-02 20:23 ` Johannes Berg
2017-12-02 20:23 ` [PATCH net 2/2] nl80211: use NLA_U8_BUGGY for two attributes Johannes Berg
2017-12-02 22:48 ` [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type David Ahern
2017-12-02 22:48   ` David Ahern
2017-12-05 16:31 ` David Miller
2017-12-05 16:34   ` Johannes Berg
2017-12-05 16:34     ` Johannes Berg
2017-12-05 16:41     ` David Miller
2017-12-05 17:30       ` Johannes Berg
2017-12-05 17:40         ` David Miller
2017-12-05 18:15           ` David Ahern
2017-12-05 18:15             ` David Ahern
2017-12-05 19:08             ` David Miller
2017-12-05 16:41     ` David Ahern
2017-12-05 16:51       ` David Miller
2017-12-05 16:51         ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.