Re: [RFC v2] mac80211: fix possible null-pointer dereference

[Johannes Berg <johannes@sipsolutions.net>]

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