2009-07-20 08:11:36

by Zhu Yi

[permalink] [raw]
Subject: [PATCH] cfg80211: avoid setting default_key if add_key fails

In cfg80211_upload_connect_keys(), we call add_key, set_default_key
and set_default_mgmt_key (if applicable) one by one. If one of these
operations fails, we should stop calling the following functions.
Because if the key is not added successfully, we should not set it as
default key anyway.

Signed-off-by: Zhu Yi <[email protected]>
---
net/wireless/util.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 4bab380..ba387d8 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -546,13 +546,17 @@ void cfg80211_upload_connect_keys(struct wireless_dev *wdev)
if (!wdev->connect_keys->params[i].cipher)
continue;
if (rdev->ops->add_key(wdev->wiphy, dev, i, NULL,
- &wdev->connect_keys->params[i]))
+ &wdev->connect_keys->params[i])) {
printk(KERN_ERR "%s: failed to set key %d\n",
dev->name, i);
+ continue;
+ }
if (wdev->connect_keys->def == i)
- if (rdev->ops->set_default_key(wdev->wiphy, dev, i))
+ if (rdev->ops->set_default_key(wdev->wiphy, dev, i)) {
printk(KERN_ERR "%s: failed to set defkey %d\n",
dev->name, i);
+ continue;
+ }
if (wdev->connect_keys->defmgmt == i)
if (rdev->ops->set_default_mgmt_key(wdev->wiphy, dev, i))
printk(KERN_ERR "%s: failed to set mgtdef %d\n",
--
1.6.0.4



2009-07-20 10:34:34

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] cfg80211: avoid setting default_key if add_key fails

On Mon, 2009-07-20 at 16:12 +0800, Zhu Yi wrote:
> In cfg80211_upload_connect_keys(), we call add_key, set_default_key
> and set_default_mgmt_key (if applicable) one by one. If one of these
> operations fails, we should stop calling the following functions.
> Because if the key is not added successfully, we should not set it as
> default key anyway.

Should we even disconnect again in that case?

johannes

> Signed-off-by: Zhu Yi <[email protected]>
> ---
> net/wireless/util.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 4bab380..ba387d8 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -546,13 +546,17 @@ void cfg80211_upload_connect_keys(struct wireless_dev *wdev)
> if (!wdev->connect_keys->params[i].cipher)
> continue;
> if (rdev->ops->add_key(wdev->wiphy, dev, i, NULL,
> - &wdev->connect_keys->params[i]))
> + &wdev->connect_keys->params[i])) {
> printk(KERN_ERR "%s: failed to set key %d\n",
> dev->name, i);
> + continue;
> + }
> if (wdev->connect_keys->def == i)
> - if (rdev->ops->set_default_key(wdev->wiphy, dev, i))
> + if (rdev->ops->set_default_key(wdev->wiphy, dev, i)) {
> printk(KERN_ERR "%s: failed to set defkey %d\n",
> dev->name, i);
> + continue;
> + }
> if (wdev->connect_keys->defmgmt == i)
> if (rdev->ops->set_default_mgmt_key(wdev->wiphy, dev, i))
> printk(KERN_ERR "%s: failed to set mgtdef %d\n",


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2009-07-21 10:44:14

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] cfg80211: avoid setting default_key if add_key fails

On Tue, 2009-07-21 at 09:47 +0800, Zhu Yi wrote:

> > Should we even disconnect again in that case?
>
> Yeah, if one key setting is failed, it might not be worth trying others.

Well, even if the others don't fail, the connection wouldn't be working
properly in that case.

> This is true for iwmc3200wifi. But I cannot speak for other drivers. How
> about merge this patch first? This is clearly a bug because the firmware
> confused when it is told to set a key as default but never existed.

Agree, let's merge this first and then see how to disconnect.

johannes


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2009-07-21 01:47:34

by Zhu Yi

[permalink] [raw]
Subject: Re: [PATCH] cfg80211: avoid setting default_key if add_key fails

On Mon, 2009-07-20 at 18:34 +0800, Johannes Berg wrote:
> On Mon, 2009-07-20 at 16:12 +0800, Zhu Yi wrote:
> > In cfg80211_upload_connect_keys(), we call add_key, set_default_key
> > and set_default_mgmt_key (if applicable) one by one. If one of these
> > operations fails, we should stop calling the following functions.
> > Because if the key is not added successfully, we should not set it as
> > default key anyway.
>
> Should we even disconnect again in that case?

Yeah, if one key setting is failed, it might not be worth trying others.
This is true for iwmc3200wifi. But I cannot speak for other drivers. How
about merge this patch first? This is clearly a bug because the firmware
confused when it is told to set a key as default but never existed.

Thanks,
-yi

> > Signed-off-by: Zhu Yi <[email protected]>
> > ---
> > net/wireless/util.c | 8 ++++++--
> > 1 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/wireless/util.c b/net/wireless/util.c
> > index 4bab380..ba387d8 100644
> > --- a/net/wireless/util.c
> > +++ b/net/wireless/util.c
> > @@ -546,13 +546,17 @@ void cfg80211_upload_connect_keys(struct wireless_dev *wdev)
> > if (!wdev->connect_keys->params[i].cipher)
> > continue;
> > if (rdev->ops->add_key(wdev->wiphy, dev, i, NULL,
> > - &wdev->connect_keys->params[i]))
> > + &wdev->connect_keys->params[i])) {
> > printk(KERN_ERR "%s: failed to set key %d\n",
> > dev->name, i);
> > + continue;
> > + }
> > if (wdev->connect_keys->def == i)
> > - if (rdev->ops->set_default_key(wdev->wiphy, dev, i))
> > + if (rdev->ops->set_default_key(wdev->wiphy, dev, i)) {
> > printk(KERN_ERR "%s: failed to set defkey %d\n",
> > dev->name, i);
> > + continue;
> > + }
> > if (wdev->connect_keys->defmgmt == i)
> > if (rdev->ops->set_default_mgmt_key(wdev->wiphy, dev, i))
> > printk(KERN_ERR "%s: failed to set mgtdef %d\n",