net/mac80211/mesh_plink.c +574 mesh_rx_plink_frame(168)
error: we previously assumed 'sta' could be null.
This bug was detected by smatch.
( http://repo.or.cz/w/smatch.git )
Cc: <[email protected]>
Signed-off-by: Christian Lamparter <[email protected]>
---
diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index ea13a80..1d7c564 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -473,7 +473,7 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata, struct ieee80211_m
rcu_read_lock();
sta = sta_info_get(sdata, mgmt->sa);
- if (!sta && ftype != PLINK_OPEN) {
+ if (!sta || ftype != PLINK_OPEN) {
mpl_dbg("Mesh plink: cls or cnf from unknown peer\n");
rcu_read_unlock();
return;
On Sat, Sep 25, 2010 at 12:02:20AM +0200, Christian Lamparter wrote:
> because mesh uses actions instead of AUTH/ASSOC and
> the following code in ieee80211_rx_h_action (rx.c)
>
> 1986) if (!rx->sta && mgmt->u.action.category != WLAN_CATEGORY_PUBLIC)
> 1987) return RX_DROP_UNUSABLE;
>
> prevents any new plinks because action.category is probably
> WLAN_CATEGORY_MESH_PLINK, right?
Which Category does not even exist in the latest P802.11s draft..
Someone needs to update the mesh code to match with the latest draft at
some point and the processing of Action frames for setup (between two
STAs that are not associated) needs some changes (e.g., handling of the
new Self Protect Action Category).
--
Jouni Malinen PGP id EFC895FA
On Fri, Sep 24, 2010 at 6:02 PM, Christian Lamparter
<[email protected]> wrote:
> hard to say, smatch must see the null dereference, when
> we receive an action action frame where ftype is PLINK_OPEN
> and the mesh_matches_local(&elems, sdata) check fail, but why
> doesn't it complain about the "spin_lock_bh(&sta->lock)"?
Smatch just does pattern matching right? Maybe smatch doesn't
assume you are actually using the pointer in spin_lock_bh().
I.e. it is ok to do "&null_ptr->member", offsetof() basically
does that; but not "null_ptr->member".
--
Bob Copeland %% http://www.bobcopeland.com
On Tue, Sep 21, 2010 at 12:57:13AM +0200, Christian Lamparter wrote:
> net/mac80211/mesh_plink.c +574 mesh_rx_plink_frame(168)
> error: we previously assumed 'sta' could be null.
>
> This bug was detected by smatch.
> ( http://repo.or.cz/w/smatch.git )
>
> Cc: <[email protected]>
> Signed-off-by: Christian Lamparter <[email protected]>
> ---
> diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
> index ea13a80..1d7c564 100644
> --- a/net/mac80211/mesh_plink.c
> +++ b/net/mac80211/mesh_plink.c
> @@ -473,7 +473,7 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata, struct ieee80211_m
> rcu_read_lock();
>
> sta = sta_info_get(sdata, mgmt->sa);
> - if (!sta && ftype != PLINK_OPEN) {
> + if (!sta || ftype != PLINK_OPEN) {
> mpl_dbg("Mesh plink: cls or cnf from unknown peer\n");
> rcu_read_unlock();
> return;
Are you sure this is the intended check? It isn't clear to me from looking at the code.
Perhaps line 574 just needs to be protected by another NULL check?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Friday 24 September 2010 20:00:13 John W. Linville wrote:
> On Tue, Sep 21, 2010 at 12:57:13AM +0200, Christian Lamparter wrote:
> > net/mac80211/mesh_plink.c +574 mesh_rx_plink_frame(168)
> > error: we previously assumed 'sta' could be null.
> >
> > This bug was detected by smatch.
> > ( http://repo.or.cz/w/smatch.git )
> >
> > Cc: <[email protected]>
> > Signed-off-by: Christian Lamparter <[email protected]>
> > ---
> > diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
> > index ea13a80..1d7c564 100644
> > --- a/net/mac80211/mesh_plink.c
> > +++ b/net/mac80211/mesh_plink.c
> > @@ -473,7 +473,7 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata, struct ieee80211_m
> > rcu_read_lock();
> >
> > sta = sta_info_get(sdata, mgmt->sa);
> > - if (!sta && ftype != PLINK_OPEN) {
> > + if (!sta || ftype != PLINK_OPEN) {
> > mpl_dbg("Mesh plink: cls or cnf from unknown peer\n");
> > rcu_read_unlock();
> > return;
>
> Are you sure this is the intended check? It isn't clear to me from looking at the code.
You are right, that change may even break mesh.
(if it did work?)
because mesh uses actions instead of AUTH/ASSOC and
the following code in ieee80211_rx_h_action (rx.c)
1986) if (!rx->sta && mgmt->u.action.category != WLAN_CATEGORY_PUBLIC)
1987) return RX_DROP_UNUSABLE;
prevents any new plinks because action.category is probably
WLAN_CATEGORY_MESH_PLINK, right?
> Perhaps line 574 just needs to be protected by another NULL check?
hard to say, smatch must see the null dereference, when
we receive an action action frame where ftype is PLINK_OPEN
and the mesh_matches_local(&elems, sdata) check fail, but why
doesn't it complain about the "spin_lock_bh(&sta->lock)"?
---
[...]
(from previous checks we know that sta has to be pointing to a
valid sta_info or ftype is set to PLINK_OPEN)
if (ftype != PLINK_CLOSE && (!mesh_matches_local(&elems, sdata))) {
switch (ftype) {
case PLINK_OPEN:
event = OPN_RJCT;
break;
case PLINK_CONFIRM:
event = CNF_RJCT;
break;
case PLINK_CLOSE:
/* avoid warning */
break;
}
--> spin_lock_bh(&sta->lock); <--
} else if (!sta) {
/* ftype == PLINK_OPEN */
...
}
...
---
anyway here's my new fix (this time RFC). Is there anyone at
cozybits who can review the logic in the new patch?
---
diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index ea13a80..8b7efee 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -412,7 +412,7 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata, struct ieee80211_m
enum plink_event event;
enum plink_frame_type ftype;
size_t baselen;
- bool deactivated;
+ bool deactivated, matches_local = true;
u8 ie_len;
u8 *baseaddr;
__le16 plid, llid, reason;
@@ -487,6 +487,7 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata, struct ieee80211_m
/* Now we will figure out the appropriate event... */
event = PLINK_UNDEFINED;
if (ftype != PLINK_CLOSE && (!mesh_matches_local(&elems, sdata))) {
+ matches_local = false;
switch (ftype) {
case PLINK_OPEN:
event = OPN_RJCT;
@@ -498,7 +499,15 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata, struct ieee80211_m
/* avoid warning */
break;
}
- spin_lock_bh(&sta->lock);
+ }
+
+ if (!sta && !matches_local) {
+ rcu_read_unlock();
+ reason = cpu_to_le16(MESH_CAPABILITY_POLICY_VIOLATION);
+ llid = 0;
+ mesh_plink_frame_tx(sdata, PLINK_CLOSE, mgmt->sa, llid,
+ plid, reason);
+ return;
} else if (!sta) {
/* ftype == PLINK_OPEN */
u32 rates;
@@ -522,7 +531,7 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata, struct ieee80211_m
}
event = OPN_ACPT;
spin_lock_bh(&sta->lock);
- } else {
+ } else if (matches_local) {
spin_lock_bh(&sta->lock);
switch (ftype) {
case PLINK_OPEN:
@@ -564,6 +573,8 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata, struct ieee80211_m
rcu_read_unlock();
return;
}
+ } else {
+ spin_lock_bh(&sta->lock);
}
mpl_dbg("Mesh plink (peer, state, llid, plid, event): %pM %s %d %d %d\n",
---
Regards,
Chr
On Thursday 30 September 2010 18:27:08 Bob Copeland wrote:
> On Fri, Sep 24, 2010 at 6:02 PM, Christian Lamparter
> <[email protected]> wrote:
>
> > hard to say, smatch must see the null dereference, when
> > we receive an action action frame where ftype is PLINK_OPEN
> > and the mesh_matches_local(&elems, sdata) check fail, but why
> > doesn't it complain about the "spin_lock_bh(&sta->lock)"?
>
> Smatch just does pattern matching right?
Uhh, I guess that's a question for Dan.
The README-smatch sums it up as:
"It's basically a state machine that tracks the flow of code."
(I think coccicheck, is the "pattern matching" checker, right?)
> Maybe smatch doesn't assume you are actually using
> the pointer in spin_lock_bh().
>
> I.e. it is ok to do "&null_ptr->member", offsetof() basically
> does that; but not "null_ptr->member".
net/mac80211/mesh_plink.c +574 mesh_rx_plink_frame(168)
error: we previously assumed 'sta' could be null.
574: switch (sta->plink_state) {
Smatch is definitely following code paths. Is there a switch
to make it more verbose (e.g.: comment on about the conditions
about the objected code - branch)?
Regards,
Chr
Johannes,
On Thu, Oct 7, 2010 at 3:54 PM, Johannes Berg <[email protected]> wrote:
>> Javier and I reviewed the patch and it definitely fixes a potential
>> problem and is correct. ?Furthermore, applied to wireless-testing
>> head, it passes all of our cases in our test bed.
>>
>> I think it's good to go.
>
> Err, are you positive? I think the code there is correct, apart from the
> fact that it does no validation of
> mgmt->u.action.u.plink_action.action_code whatsoever which may allow all
> kinds of abuse :)
>
> The only action that's valid w/o having a station entry for the peer is
> PLINK_OPEN, which makes perfect sense.
We believe Christian's second patch fixes a null-pointer de-reference
that would be triggered by a PLINK_OPEN frame with
mismatching/incompatible mesh configuration. Let's analyze that case:
void mesh_rx_plink_frame(...)
(...)
sta = sta_info_get(sdata, mgmt->sa); <-- will return null
if (!sta && ftype != PLINK_OPEN) { <-- false for PLINK_OPEN frames
(...)
if (ftype != PLINK_CLOSE && (!mesh_matches_local(&elems, sdata))) {
<-- true for PLINK_OPEN, non-compatible mesh config
(...)
spin_lock_bh(&sta->lock); <-- boom!
The patch not only solves this problem, but also responds correctly to
non-compatible PLINK_OPEN frames by generating a PLINK_CLOSE with the
right reason code.
Cheers,
Javier
On Fri, Sep 24, 2010 at 3:02 PM, Christian Lamparter
<[email protected]> wrote:
> ---
> anyway here's my new fix (this time RFC). Is there anyone at
> cozybits who can review the logic in the new patch?
>
Christian,
Javier and I reviewed the patch and it definitely fixes a potential
problem and is correct. Furthermore, applied to wireless-testing
head, it passes all of our cases in our test bed.
I think it's good to go.
- Steve
On Thu, Sep 30, 2010 at 06:52:30PM +0200, Christian Lamparter wrote:
> On Thursday 30 September 2010 18:27:08 Bob Copeland wrote:
> > On Fri, Sep 24, 2010 at 6:02 PM, Christian Lamparter
> > <[email protected]> wrote:
> >
> > > hard to say, smatch must see the null dereference, when
> > > we receive an action action frame where ftype is PLINK_OPEN
> > > and the mesh_matches_local(&elems, sdata) check fail, but why
> > > doesn't it complain about the "spin_lock_bh(&sta->lock)"?
> >
> > Smatch just does pattern matching right?
> Uhh, I guess that's a question for Dan.
>
> The README-smatch sums it up as:
> "It's basically a state machine that tracks the flow of code."
>
> (I think coccicheck, is the "pattern matching" checker, right?)
> > Maybe smatch doesn't assume you are actually using
> > the pointer in spin_lock_bh().
> >
> > I.e. it is ok to do "&null_ptr->member", offsetof() basically
> > does that; but not "null_ptr->member".
>
Yes. You are right.
This is from check_check_deref.c which handles this as a special case
because people quite often do:
struct foo *bar = &x->y;
if (!x)
return;
If you comment out the "if (getting_address())" check then it will
complain.
> net/mac80211/mesh_plink.c +574 mesh_rx_plink_frame(168)
> error: we previously assumed 'sta' could be null.
>
> 574: switch (sta->plink_state) {
>
> Smatch is definitely following code paths. Is there a switch
> to make it more verbose (e.g.: comment on about the conditions
> about the objected code - branch)?
There is a --debug but I suspect it's way more verbose than what you
want.
You could also hack the net/mac80211/mesh_plink.c source file and add
an "#include /path/to/smatch/check_debug.h" and sprinkle the code with
calls to __smatch_cur_slist() which will make it dump all the current
states at that point.
regards,
dan carpenter
Johannes
On Fri, Oct 8, 2010 at 11:03 AM, Johannes Berg
<[email protected]> wrote:
>> The patch not only solves this problem, but also responds correctly to
>> non-compatible PLINK_OPEN frames by generating a PLINK_CLOSE with the
>> right reason code.
>
> But then you can't ever actually properly process a *matching*
> PLINK_OPEN frame, afaict, because those definitely do have
> ? ? ? ?"!sta && ftype == PLINK_OPEN"
Not always: an sta can be created on reception of a beacon or probe
response (mesh_neighbour_update()), so you could have ftype ==
PLINK_OPEN and a non-null sta. Buy, yes, I agree that !sta && ftype ==
PLINK_OPEN is a valid case.
> which is how the "if (!sta && ftype != PLINK_OPEN) return" came about,
> afaict.
Yes, and Christian's *second* patch (submitted as RFC on Sept 24) does
not change that check. The matching PLINK_OPEN would be handled in
the following branch:
} else if (!sta) {
/* ftype == PLINK_OPEN */
u32 rates;
(...)
sta = mesh_plink_alloc(sdata, mgmt->sa, rates);
> So I still don't think it's quite correct to fix it this way.
Could it be that we are looking at different patches? Just to be
sure, let me paste below the one we are referring to as sent by
Christian:
---
diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index ea13a80..8b7efee 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -412,7 +412,7 @@ void mesh_rx_plink_frame(struct
ieee80211_sub_if_data *sdata, struct ieee80211_m
enum plink_event event;
enum plink_frame_type ftype;
size_t baselen;
- bool deactivated;
+ bool deactivated, matches_local = true;
u8 ie_len;
u8 *baseaddr;
__le16 plid, llid, reason;
@@ -487,6 +487,7 @@ void mesh_rx_plink_frame(struct
ieee80211_sub_if_data *sdata, struct ieee80211_m
/* Now we will figure out the appropriate event... */
event = PLINK_UNDEFINED;
if (ftype != PLINK_CLOSE && (!mesh_matches_local(&elems, sdata))) {
+ matches_local = false;
switch (ftype) {
case PLINK_OPEN:
event = OPN_RJCT;
@@ -498,7 +499,15 @@ void mesh_rx_plink_frame(struct
ieee80211_sub_if_data *sdata, struct ieee80211_m
/* avoid warning */
break;
}
- spin_lock_bh(&sta->lock);
+ }
+
+ if (!sta && !matches_local) {
+ rcu_read_unlock();
+ reason = cpu_to_le16(MESH_CAPABILITY_POLICY_VIOLATION);
+ llid = 0;
+ mesh_plink_frame_tx(sdata, PLINK_CLOSE, mgmt->sa, llid,
+ plid, reason);
+ return;
} else if (!sta) {
/* ftype == PLINK_OPEN */
u32 rates;
@@ -522,7 +531,7 @@ void mesh_rx_plink_frame(struct
ieee80211_sub_if_data *sdata, struct ieee80211_m
}
event = OPN_ACPT;
spin_lock_bh(&sta->lock);
- } else {
+ } else if (matches_local) {
spin_lock_bh(&sta->lock);
switch (ftype) {
case PLINK_OPEN:
@@ -564,6 +573,8 @@ void mesh_rx_plink_frame(struct
ieee80211_sub_if_data *sdata, struct ieee80211_m
rcu_read_unlock();
return;
}
+ } else {
+ spin_lock_bh(&sta->lock);
}
mpl_dbg("Mesh plink (peer, state, llid, plid, event): %pM %s %d %d %d\n",
---
Cheers,
Javier
On Fri, 2010-10-08 at 10:56 -0700, Javier Cardona wrote:
> We believe Christian's second patch fixes a null-pointer de-reference
> that would be triggered by a PLINK_OPEN frame with
> mismatching/incompatible mesh configuration. Let's analyze that case:
>
> void mesh_rx_plink_frame(...)
> (...)
> sta = sta_info_get(sdata, mgmt->sa); <-- will return null
> if (!sta && ftype != PLINK_OPEN) { <-- false for PLINK_OPEN frames
> (...)
> if (ftype != PLINK_CLOSE && (!mesh_matches_local(&elems, sdata))) {
> <-- true for PLINK_OPEN, non-compatible mesh config
> (...)
> spin_lock_bh(&sta->lock); <-- boom!
Good point. I glossed over the part here and just looked at the else
branch with !sta.
> The patch not only solves this problem, but also responds correctly to
> non-compatible PLINK_OPEN frames by generating a PLINK_CLOSE with the
> right reason code.
But then you can't ever actually properly process a *matching*
PLINK_OPEN frame, afaict, because those definitely do have
"!sta && ftype == PLINK_OPEN"
which is how the "if (!sta && ftype != PLINK_OPEN) return" came about,
afaict.
So I still don't think it's quite correct to fix it this way.
johannes
On Thu, 2010-10-07 at 15:38 -0700, Steve deRosier wrote:
> Javier and I reviewed the patch and it definitely fixes a potential
> problem and is correct. Furthermore, applied to wireless-testing
> head, it passes all of our cases in our test bed.
>
> I think it's good to go.
Err, are you positive? I think the code there is correct, apart from the
fact that it does no validation of
mgmt->u.action.u.plink_action.action_code whatsoever which may allow all
kinds of abuse :)
The only action that's valid w/o having a station entry for the peer is
PLINK_OPEN, which makes perfect sense.
johannes
On Fri, 2010-10-08 at 11:25 -0700, Javier Cardona wrote:
> > But then you can't ever actually properly process a *matching*
> > PLINK_OPEN frame, afaict, because those definitely do have
> > "!sta && ftype == PLINK_OPEN"
>
> Not always: an sta can be created on reception of a beacon or probe
> response (mesh_neighbour_update()), so you could have ftype ==
> PLINK_OPEN and a non-null sta. Buy, yes, I agree that !sta && ftype ==
> PLINK_OPEN is a valid case.
Ah, indeed.
> > which is how the "if (!sta && ftype != PLINK_OPEN) return" came about,
> > afaict.
>
> Yes, and Christian's *second* patch (submitted as RFC on Sept 24) does
> not change that check. The matching PLINK_OPEN would be handled in
> the following branch:
>
> } else if (!sta) {
> /* ftype == PLINK_OPEN */
> u32 rates;
> (...)
> sta = mesh_plink_alloc(sdata, mgmt->sa, rates);
>
> > So I still don't think it's quite correct to fix it this way.
>
> Could it be that we are looking at different patches? Just to be
> sure, let me paste below the one we are referring to as sent by
> Christian:
Ahrg, yes, I always just went back to his first patch.
> ---
> diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
> index ea13a80..8b7efee 100644
> --- a/net/mac80211/mesh_plink.c
> +++ b/net/mac80211/mesh_plink.c
> @@ -412,7 +412,7 @@ void mesh_rx_plink_frame(struct
> ieee80211_sub_if_data *sdata, struct ieee80211_m
> enum plink_event event;
> enum plink_frame_type ftype;
> size_t baselen;
> - bool deactivated;
> + bool deactivated, matches_local = true;
> u8 ie_len;
> u8 *baseaddr;
> __le16 plid, llid, reason;
> @@ -487,6 +487,7 @@ void mesh_rx_plink_frame(struct
> ieee80211_sub_if_data *sdata, struct ieee80211_m
> /* Now we will figure out the appropriate event... */
> event = PLINK_UNDEFINED;
> if (ftype != PLINK_CLOSE && (!mesh_matches_local(&elems, sdata))) {
> + matches_local = false;
> switch (ftype) {
> case PLINK_OPEN:
> event = OPN_RJCT;
> @@ -498,7 +499,15 @@ void mesh_rx_plink_frame(struct
> ieee80211_sub_if_data *sdata, struct ieee80211_m
> /* avoid warning */
> break;
> }
> - spin_lock_bh(&sta->lock);
> + }
> +
> + if (!sta && !matches_local) {
> + rcu_read_unlock();
> + reason = cpu_to_le16(MESH_CAPABILITY_POLICY_VIOLATION);
> + llid = 0;
> + mesh_plink_frame_tx(sdata, PLINK_CLOSE, mgmt->sa, llid,
> + plid, reason);
> + return;
> } else if (!sta) {
> /* ftype == PLINK_OPEN */
> u32 rates;
> @@ -522,7 +531,7 @@ void mesh_rx_plink_frame(struct
> ieee80211_sub_if_data *sdata, struct ieee80211_m
> }
> event = OPN_ACPT;
> spin_lock_bh(&sta->lock);
> - } else {
> + } else if (matches_local) {
> spin_lock_bh(&sta->lock);
> switch (ftype) {
> case PLINK_OPEN:
> @@ -564,6 +573,8 @@ void mesh_rx_plink_frame(struct
> ieee80211_sub_if_data *sdata, struct ieee80211_m
> rcu_read_unlock();
> return;
> }
> + } else {
> + spin_lock_bh(&sta->lock);
> }
Yeah, this looks good. Sorry!
johannes