| Message ID | 1323407410-25072-1-git-send-email-jungwalk@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, 2011-12-09 at 13:10 +0800, jungwalk@gmail.com wrote: > From: Spencer Chang <jungwalk@gmail.com> > > It is better to reset the ssid in WPS first before it is replaced with the one in > credential in case the length of ssid in credential is longer than wps->ssid. > + os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN); > os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len); > hapd->wps->ssid_len = cred->ssid_len; That seems completely useless, in particular when it's longer since then it will partially overwrite it anyway. They are binary data, not strings, after all. johannes
Oops, typo. What I wanted to say is "in case the length of ssid in wps->ssid is longer than the one in credential". Then, does this make more sense? If it is, I will resend the patch. 2011/12/9 Johannes Berg <johannes@sipsolutions.net> > On Fri, 2011-12-09 at 13:10 +0800, jungwalk@gmail.com wrote: > > From: Spencer Chang <jungwalk@gmail.com> > > > > It is better to reset the ssid in WPS first before it is replaced with > the one in > > credential in case the length of ssid in credential is longer than > wps->ssid. > > > + os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN); > > os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len); > > hapd->wps->ssid_len = cred->ssid_len; > > That seems completely useless, in particular when it's longer since then > it will partially overwrite it anyway. They are binary data, not > strings, after all. > > johannes > >
On Fri, 2011-12-09 at 21:31 +0800, 張晏榕 wrote: > Oops, typo. > > What I wanted to say is "in case the length of ssid in wps->ssid is > longer than the one in credential". > Then, does this make more sense? > If it is, I will resend the patch. Even then, it doesn't seem to make a lot of sense, since not setting it to 0 will only cause some old data to be left in an unused portion of the memory ... johannes
On Fri, Dec 09, 2011 at 01:10:10PM +0800, jungwalk@gmail.com wrote: > It is better to reset the ssid in WPS first before it is replaced with the one in > credential in case the length of ssid in credential is longer than wps->ssid. Are you trying to fix a bug that addresses some noticeable issue? > @@ -318,6 +318,7 @@ static int hapd_wps_cred_cb(struct hostapd_data *hapd, void *ctx) > + os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN); > os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len); > hapd->wps->ssid_len = cred->ssid_len; Like Johannes pointed out, this should not really result in any real change in actual behavior. If it does, there is likely a real bug somewhere else and that should be fixed. The SSID buffer is used as an arbitrary binary array of ssid_len octets and as such, there is no need to null-terminate it - the extra octets in the buffer are ignored.
Hi Jouni and Johannes, Yes, I am trying to fix a bug related to generating random WPS ssid for each band, and thought this kind of change maybe necessary even without that random WPS ssid modification. However, I seems to misunderstand how to use that SSID buffer. I will check it more. Thanks for your responses. :) 2011/12/10 Jouni Malinen <j@w1.fi> > On Fri, Dec 09, 2011 at 01:10:10PM +0800, jungwalk@gmail.com wrote: > > It is better to reset the ssid in WPS first before it is replaced with > the one in > > credential in case the length of ssid in credential is longer than > wps->ssid. > > Are you trying to fix a bug that addresses some noticeable issue? > > > @@ -318,6 +318,7 @@ static int hapd_wps_cred_cb(struct hostapd_data > *hapd, void *ctx) > > + os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN); > > os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len); > > hapd->wps->ssid_len = cred->ssid_len; > > Like Johannes pointed out, this should not really result in any real > change in actual behavior. If it does, there is likely a real bug > somewhere else and that should be fixed. The SSID buffer is used as an > arbitrary binary array of ssid_len octets and as such, there is no need > to null-terminate it - the extra octets in the buffer are ignored. > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap >
On Sat, Dec 10, 2011 at 05:50:46AM +0800, 張晏榕 wrote: > Yes, I am trying to fix a bug related to generating random WPS ssid for > each band, and thought this kind of change maybe necessary even without > that random WPS ssid modification. Could you please describe that in more detail? That sounds more like a misunderstanding of how hostapd is supposed to work since hostapd does not generate a random SSID. It does generate a random passphrase when moving from unconfigured to configured state, but the initial SSID is assumed to be generated by something external.
Hi Jouni, Sorry for misleading. That code of generating random SSID is added by myself for customer's request, and I misunderstood how the SSID buffer should be used. Therefore, I created an issue of wrong SSID on dual band product and that issue could be fixed with that patch of resetting SSID buffer. And then I thought may be the SSID buffer should be reset even in original hostapd code (without random SSID generation). But I am wrong. After you and Johannes described, I know the root cause of that wrong SSID issue I created and it should be fixed by following the right way of using SSID buffer, and it is not necessary to reset the SSID buffer. Thanks for your responses and sorry for misleading again. 2011/12/10 Jouni Malinen <j@w1.fi> > On Sat, Dec 10, 2011 at 05:50:46AM +0800, 張晏榕 wrote: > > Yes, I am trying to fix a bug related to generating random WPS ssid for > > each band, and thought this kind of change maybe necessary even without > > that random WPS ssid modification. > > Could you please describe that in more detail? That sounds more like a > misunderstanding of how hostapd is supposed to work since hostapd does > not generate a random SSID. It does generate a random passphrase when > moving from unconfigured to configured state, but the initial SSID is > assumed to be generated by something external. > > -- > Jouni Malinen PGP id EFC895FA > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap >
diff --git a/src/ap/wps_hostapd.c b/src/ap/wps_hostapd.c index 817012e..42b22a2 100644 --- a/src/ap/wps_hostapd.c +++ b/src/ap/wps_hostapd.c @@ -318,6 +318,7 @@ static int hapd_wps_cred_cb(struct hostapd_data *hapd, void *ctx) if (hapd->conf->wps_cred_processing == 1) return 0; + os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN); os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len); hapd->wps->ssid_len = cred->ssid_len; hapd->wps->encr_types = cred->encr_type;