[PATCH] regulatory: add NULL to alpha2

[PATCH] regulatory: add NULL to alpha2

[Arik Nemtsov]

From: Eliad Peller <eliad@wizery.com>

alpha2 is defined as 2-chars array, but is used in multiple
places as string (e.g. with nla_put_string calls), which
might leak kernel data.

Solve it by simply adding an extra char for the NULL
terminator, making such operations safe.

Signed-off-by: Eliad Peller <eliadx.peller@intel.com>
Reviewed-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/regulatory.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/regulatory.h b/include/net/regulatory.h
index 2599924..dad7ab2 100644
--- a/include/net/regulatory.h
+++ b/include/net/regulatory.h
@@ -167,7 +167,7 @@ struct ieee80211_reg_rule {
 struct ieee80211_regdomain {
 	struct rcu_head rcu_head;
 	u32 n_reg_rules;
-	char alpha2[2];
+	char alpha2[3];
 	enum nl80211_dfs_regions dfs_region;
 	struct ieee80211_reg_rule reg_rules[];
 };
-- 
1.9.1

Re: [PATCH] regulatory: add NULL to alpha2

[Luis R. Rodriguez]

On Wed, Jun 11, 2014 at 12:23 AM, Arik Nemtsov <arik@wizery.com> wrote:
> From: Eliad Peller <eliad@wizery.com>
>
> alpha2 is defined as 2-chars array, but is used in multiple
> places as string (e.g. with nla_put_string calls), which
> might leak kernel data.
>
> Solve it by simply adding an extra char for the NULL
> terminator, making such operations safe.
>
> Signed-off-by: Eliad Peller <eliadx.peller@intel.com>
> Reviewed-by: Johannes Berg <johannes.berg@intel.com>

Acked-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>

Thanks for the patch ! Curious how you ended up down finding the issue though.

  Luis

Re: [PATCH] regulatory: add NULL to alpha2

[Eliad Peller]

On Wed, Jun 11, 2014 at 11:30 AM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> On Wed, Jun 11, 2014 at 12:23 AM, Arik Nemtsov <arik@wizery.com> wrote:
>> From: Eliad Peller <eliad@wizery.com>
>>
>> alpha2 is defined as 2-chars array, but is used in multiple
>> places as string (e.g. with nla_put_string calls), which
>> might leak kernel data.
>>
>> Solve it by simply adding an extra char for the NULL
>> terminator, making such operations safe.
>>
>> Signed-off-by: Eliad Peller <eliadx.peller@intel.com>
>> Reviewed-by: Johannes Berg <johannes.berg@intel.com>
>
> Acked-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
>
> Thanks for the patch ! Curious how you ended up down finding the issue though.
>
i'm not sure. i think i noticed some crap in hostapd log and looked for it.

Eliad.

Re: [PATCH] regulatory: add NULL to alpha2

[Johannes Berg]

On Wed, 2014-06-11 at 10:23 +0300, Arik Nemtsov wrote:
> From: Eliad Peller <eliad@wizery.com>
> 
> alpha2 is defined as 2-chars array, but is used in multiple
> places as string (e.g. with nla_put_string calls), which
> might leak kernel data.
> 
> Solve it by simply adding an extra char for the NULL
> terminator, making such operations safe.

I'm beginning to think that it would make more sense to just not use
nla_put_string()?

johannes

Re: [PATCH] regulatory: add NULL to alpha2

[Luis R. Rodriguez]

On Mon, Jun 23, 2014 at 11:43:06AM +0200, Johannes Berg wrote:
> On Wed, 2014-06-11 at 10:23 +0300, Arik Nemtsov wrote:
> > From: Eliad Peller <eliad@wizery.com>
> > 
> > alpha2 is defined as 2-chars array, but is used in multiple
> > places as string (e.g. with nla_put_string calls), which
> > might leak kernel data.
> > 
> > Solve it by simply adding an extra char for the NULL
> > terminator, making such operations safe.
> 
> I'm beginning to think that it would make more sense to just not use
> nla_put_string()?

If the reason for adding the check was because of a new use case
then yes, but other than that are there other reasons you were
considering?

 Luis

Re: [PATCH] regulatory: add NULL to alpha2

[Johannes Berg]

On Mon, 2014-06-30 at 15:23 -0700, Luis R. Rodriguez wrote:
> On Mon, Jun 23, 2014 at 11:43:06AM +0200, Johannes Berg wrote:
> > On Wed, 2014-06-11 at 10:23 +0300, Arik Nemtsov wrote:
> > > From: Eliad Peller <eliad@wizery.com>
> > > 
> > > alpha2 is defined as 2-chars array, but is used in multiple
> > > places as string (e.g. with nla_put_string calls), which
> > > might leak kernel data.
> > > 
> > > Solve it by simply adding an extra char for the NULL
> > > terminator, making such operations safe.
> > 
> > I'm beginning to think that it would make more sense to just not use
> > nla_put_string()?
> 
> If the reason for adding the check was because of a new use case
> then yes, but other than that are there other reasons you were
> considering?

This was never intended to be a string, only 2 characters ... so why use
it with string functions?

johannes

Re: [PATCH] regulatory: add NULL to alpha2

[Luis R. Rodriguez]

On Mon, Jul 21, 2014 at 3:02 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> This was never intended to be a string, only 2 characters ... so why use
> it with string functions?

Sure makes sense, the report was not clear so indeed, if userspace can
be fixed that's better to enable consistent use.

  Luis

Re: [PATCH] regulatory: add NULL to alpha2

[Johannes Berg]

On Mon, 2014-07-28 at 18:26 -0700, Luis R. Rodriguez wrote:
> On Mon, Jul 21, 2014 at 3:02 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > This was never intended to be a string, only 2 characters ... so why use
> > it with string functions?
> 
> Sure makes sense, the report was not clear so indeed, if userspace can
> be fixed that's better to enable consistent use.

Sadly, it looks like userspace *is* relying on a NUL-byte (e.g. iw using
nla_get_string()) which would always be there due to padding, but it's
probably better to make it explicit.

johannes

Re: [PATCH] regulatory: add NULL to alpha2

[Johannes Berg]

On Wed, 2014-06-11 at 10:23 +0300, Arik Nemtsov wrote:
> From: Eliad Peller <eliad@wizery.com>
> 
> alpha2 is defined as 2-chars array, but is used in multiple
> places as string (e.g. with nla_put_string calls), which
> might leak kernel data.
> 
> Solve it by simply adding an extra char for the NULL
> terminator, making such operations safe.
> 
> Signed-off-by: Eliad Peller <eliadx.peller@intel.com>


Applied.

johannes