From: Eliad Peller <[email protected]>
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 <[email protected]>
Reviewed-by: Johannes Berg <[email protected]>
---
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
On Wed, 2014-06-11 at 10:23 +0300, Arik Nemtsov wrote:
> From: Eliad Peller <[email protected]>
>
> 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
On Wed, Jun 11, 2014 at 11:30 AM, Luis R. Rodriguez
<[email protected]> wrote:
> On Wed, Jun 11, 2014 at 12:23 AM, Arik Nemtsov <[email protected]> wrote:
>> From: Eliad Peller <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: Johannes Berg <[email protected]>
>
> Acked-by: Luis R. Rodriguez <[email protected]>
>
> 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.
On Wed, Jun 11, 2014 at 12:23 AM, Arik Nemtsov <[email protected]> wrote:
> From: Eliad Peller <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Johannes Berg <[email protected]>
Acked-by: Luis R. Rodriguez <[email protected]>
Thanks for the patch ! Curious how you ended up down finding the issue though.
Luis
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 <[email protected]>
> > >
> > > 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
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 <[email protected]>
> >
> > 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
On Mon, Jul 21, 2014 at 3:02 AM, Johannes Berg
<[email protected]> 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
On Mon, 2014-07-28 at 18:26 -0700, Luis R. Rodriguez wrote:
> On Mon, Jul 21, 2014 at 3:02 AM, Johannes Berg
> <[email protected]> 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
On Wed, 2014-06-11 at 10:23 +0300, Arik Nemtsov wrote:
> From: Eliad Peller <[email protected]>
>
> 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 <[email protected]>
Applied.
johannes