Quaker,
I don't have so many comments, most of them are nits.
=============================================================================
wpa/wpa2 code review comments
Code Review Comments Records
RE Quaker Fang
CODE REVIEWER Cecilia Hu
WEBREV
webrev: http://cr.grommit.com/~zf162725/cr_0308/
workspace & cscope: /net/greatwall.prc/workspace/wifi-wpa-cr/usr/src
REVIEW FILES
usr/src/cmd/cmd-inet/usr.lib/wpad/wpa.c
usr/src/cmd/cmd-inet/usr.lib/wpad/wpa_impl.h
usr/src/cmd/cmd-inet/usr.lib/wpad/wpa_supplicant.c
REVIEW DUE DAY
2007/03/23
I. General
=============================================================================
#1. indent
I'm not quite sure it's webrev issue or source code issue. please make
sure the indent correctly.
#2. comments
More comments are always welcome. Current code is difficult to
understand by someone else the experts on security.
#3. copy/license
WPA supplicant license mentioned that README and COPYING files for
more details. Do we have these two files in source tree?
#4. code convention
Please make sure it's Sun coding style. I saw some places port from
BSD code look strange. Please make sure run cstyle checking.
II. comments on wpa_impl.h
=============================================================================
#1. why don't put all data structures in one pack(1)
#2.
216 enum {
217 WPA_DISCONNECTED, WPA_SCANNING, WPA_ASSOCIATING,
218 WPA_ASSOCIATED, WPA_4WAY_HANDSHAKE,
WPA_GROUP_HANDSHAKE,
219 WPA_COMPLETED
220 } wpa_state;
it's better to keep one entry one line;
#3.
284 void wpa_supplicant_req_scan(struct wpa_supplicant *, int, int);
285
286 void wpa_supplicant_req_auth_timeout(struct wpa_supplicant *, int,
int);
Possibly, you want to keep them in same style, like:
wpa_supplicant_req_scan();
wpa_supplicant_req_timeout();
/* I didn't see anyother timeout then auth */
III. comments on wpa.c
=============================================================================
#1.
49 static const uint8_t
50 WPA_AUTH_KEY_MGMT_UNSPEC_802_1X[] = { 0x00, 0x50, 0xf2, 1 };
51 static const uint8_t
52 WPA_AUTH_KEY_MGMT_PSK_OVER_802_1X[] = { 0x00, 0x50, 0xf2, 2 };
...
88 static const uint8_t
89 RSN_AUTH_KEY_MGMT_UNSPEC_802_1X[] = { 0x00, 0x0f, 0xac, 1 };
90 static const uint8_t
91 RSN_AUTH_KEY_MGMT_PSK_OVER_802_1X[] = { 0x00, 0x0f, 0xac, 2 };
...
313 pos += snprintf(pos, buf + len - pos,
314 "Index / AA / PMKID / expiration (in
seconds)\n");
wrong indent.
#2.
173 static int
174 wpa_selector_to_bitfield(uint8_t *s)
175 {
176 if (memcmp(s, WPA_CIPHER_SUITE_NONE, WPA_SELECTOR_LEN) == 0)
177 return (WPA_CIPHER_NONE);
178 if (memcmp(s, WPA_CIPHER_SUITE_WEP40, WPA_SELECTOR_LEN) == 0)
179 return (WPA_CIPHER_WEP40);
180 if (memcmp(s, WPA_CIPHER_SUITE_TKIP, WPA_SELECTOR_LEN) == 0)
181 return (WPA_CIPHER_TKIP);
182 if (memcmp(s, WPA_CIPHER_SUITE_CCMP, WPA_SELECTOR_LEN) == 0)
183 return (WPA_CIPHER_CCMP);
184 if (memcmp(s, WPA_CIPHER_SUITE_WEP104, WPA_SELECTOR_LEN) == 0)
185 return (WPA_CIPHER_WEP104);
186 return (0);
187 }
If cipher, by some reasons, set to the value you don't support, so far,
treat it as WPA_CIPHER_NONE, but never be checked by
wpa_parse_wpa_ie_wpa(). Or, do you need to add one value meaning
"not support"?
Other xxx_to_bitfield() functions are also have the same question.
#3.
227 static void
228 pmksa_cache_free_entry(struct wpa_supplicant *wpa_s,
229 struct rsn_pmksa_cache *entry)
230 {
231 free(entry);
232 wpa_s->pmksa_count--;
233 if (wpa_s->cur_pmksa == entry) {
234 wpa_printf(MSG_DEBUG, "RSN: removed current PMKSA
entry");
235 wpa_s->cur_pmksa = NULL;
236 }
237 }
entry is freed firstly, and then used to compare with wpa_s->cur_pmksa.
When some "streesing" testing, is it a potential problem?
#4.
265 sec = wpa_s->pmksa->expiration - time(NULL);
266 if (sec < 0)
267 sec = 0;
268 (void) eloop_register_timeout(sec + 1, 0, pmksa_cache_expire,
269 wpa_s, NULL);
the minal timeout is 1s, is it too short?
#5.
386 if (left >= WPA_SELECTOR_LEN) {
387 data->group_cipher = wpa_selector_to_bitfield(pos);
388 pos += WPA_SELECTOR_LEN;
389 left -= WPA_SELECTOR_LEN;
390 } else if (left > 0) {
391 wpa_printf(MSG_DEBUG, "%s: ie length mismatch, %u
too much",
392 "wpa_parse_wpa_ie_wpa", left);
393 return (-1);
394 }
don't you need to check the return 0 of wpa_selector_to_bitfield() which
means "not support" or "wpa_none"?
see above #2.
#6.
wpa_parse_wpa_ie_wpa()
{
...
412 } else if (left == 1) {
413 wpa_printf(MSG_DEBUG, "%s: ie too short (for key
mgmt)",
414 "wpa_parse_wpa_ie_wpa");
415 return (-1);
416 }
...
434 } else if (left == 1) {
435 wpa_printf(MSG_DEBUG, "%s: ie too short (for
capabilities)",
436 "wpa_parse_wpa_ie_wpa");
437 return (-1);
438 }
}
Question: how about left == 0. If it's acceptable case, that means "key
mgmt" and "capabilities" are both optional. is it?
same question to wpa_parse_wpa_ie_rsn().
#7.
664 return (-1);
wrong indent.
#8.
726 wpa_pmk_to_ptk(uint8_t *pmk, uint8_t *addr1, uint8_t *addr2,
727 uint8_t *nonce1, uint8_t *nonce2, uint8_t *ptk, size_t ptk_len)
728 {
729 uint8_t data[2 * IEEE80211_ADDR_LEN + 2 * 32];
32 is used many times in this subroutine. What's that mean? If there is
defination for it, maybe you want to define a macro.
#9.
904 /* RSN: msg 1/4 should contain PMKID for the
selected PMK */
905 uint8_t *pos = (uint8_t *)(key + 1);
906 uint8_t *end = pos + BE_16(key->key_data_length);
define them at the beginning of this subroutine.
#10.
982 if (wpa_s->renew_snonce) {
983 if (random_get_pseudo_bytes(wpa_s->snonce,
WPA_NONCE_LEN)) {
984 wpa_printf(MSG_WARNING, "WPA: Failed to get "
985 "random data for SNonce");
986 return;
987 }
need the free(rbuf) here.
#11.
1018 static void
1019 wpa_supplicant_process_3_of_4_gtk(struct wpa_supplicant *wpa_s,
1020 unsigned char *src_addr, struct wpa_eapol_key *key,
1021 uint8_t *gtk, int gtk_len)
The meaning of 16/32/13/5 can be eaily understood as the len for
different CIPHER, suppose they should be defined as macro somewhere in
.h file.
IV. comments on wpa_supplicant.c
=============================================================================
#1.
46 static const char *wpa_supplicant_version =
47 "wpa_supplicant v0.1";
Please make sure it's the normal policy to version a new service from v0.1.
#2.
112 * Note: new line '\n' is added to the end of the text when printing
to stdout.
This comment is not, obviously, what this subroutine actually does.
#3.
180 for (pos = ssid_txt; *pos != '\0'; pos ++) {
181 if ((uint8_t)*pos < 32 || (uint8_t)*pos >= 127)
182 *pos = '_';
183 }
you may want to add comments to explain 32/127 for ASCII.
#4.
218 (void) eloop_cancel_timeout(wpa_supplicant_scan, wpa_s, NULL);
219 (void) eloop_register_timeout(sec, usec, wpa_supplicant_scan,
220 wpa_s, NULL);
for eloop_cancel_timeout(); don't need cast it to (void);
for eloop_register_timeout(); is it not necessary to check the return
result?
#5.
314 if (bss->wpa_ie_len && bss->wpa_ie[0] == RSN_INFO_ELEM &&
315 (ssid->proto & WPA_PROTO_RSN)) {
316 wpa_printf(MSG_DEBUG, "RSN: using IEEE 802.11i/D9.0");
317 proto = WPA_PROTO_RSN;
wrong indent.
#6.
333 free(wpa_s->ap_wpa_ie);
334 wpa_s->ap_wpa_ie = malloc(ap_ie_len);
Question: I'm not quite understand the logical. can ap_ie_len change?
#7.
380 free(wpa_s->assoc_wpa_ie);
381 wpa_s->assoc_wpa_ie = NULL;
382 wpa_s->assoc_wpa_ie_len = 0;
Question: I didn't find when assoc_wpa_ie was malloc?
#8.
486 if (ssid->bssid_set &&
487 memcmp(bss->bssid, ssid->bssid,
IEEE80211_ADDR_LEN) != 0) {
488 wpa_printf(MSG_DEBUG, " skip - BSSID
mismatch");
489 }
490 if (!((ssid->proto & (WPA_PROTO_RSN |
WPA_PROTO_WPA)) &&
491 wpa_parse_wpa_ie(wpa_s, bss->wpa_ie,
492 bss->wpa_ie_len, &ie) == 0)) {
493 wpa_printf(MSG_DEBUG, " skip - "
494 "could not parse WPA/RSN IE");
495 }
496 if (!(ie.proto & ssid->proto)) {
497 wpa_printf(MSG_DEBUG, " skip - proto
mismatch");
498 }
If these three conditions will not continue the loop - means find next
bss, then it's not necessary to check them at all.
#9.
529 num = wpa_s->driver->get_scan_results(wpa_s->ifname, results,
530 MAX_SCANRESULTS);
wrong indent
#10.
554 wpa_supplicant_req_scan(wpa_s, 5, 0);
why the arguments are 5 and 0? there are someother places use these kind
of numbers, maybe need some comments...
#11.
625 static void
626 wpa_supplicant_fd_workaround(void)
627 {
628 int s, i;
629 /*
630 * When started from pcmcia-cs scripts, wpa_supplicant
might start with
631 * fd 0, 1, and 2 closed. This will cause some issues
because many
632 * places in wpa_supplicant are still printing out to
stdout. As a
633 * workaround, make sure that fd's 0, 1, and 2 are not
used for other
634 * sockets.
635 */
636 for (i = 0; i < 3; i++) {
637 s = open("/dev/null", O_RDWR);
638 if (s > 2) {
639 (void) close(s);
640 break;
641 }
642 }
643 }
Seems this workaround is copied from somehwere else, you may add
a clear comments or remove this one.
#12.
697 door_id = door_create(event_handler, cookie,
698 DOOR_UNREF | DOOR_REFUSE_DESC | DOOR_NO_CANCEL);
Question: according to the define of DOOR_NO_CANCEL. is it what the wpad
wants?
DOOR_NO_CANCEL Clients which abort calls to door_call()
on this door will not cause the cancella-
tion of the server thread handling the
request. See cancellation(5).
#13.
705 if (stat(doorname, &stbuf) < 0) {
706 int newfd;
707 ...
715 }
716
Define variable at the function beginning
#14.
750 static int
751 wpa_config_parse_ssid(struct wpa_ssid *ssid, int line, const char
*value)
752 {
the return value of this function is never be checked. why don't define
it "void"?
#15.
816 if ((ssid->key_mgmt & WPA_KEY_MGMT_PSK) && !ssid->psk_set) {
817 wpa_printf(MSG_ERROR, "WPA-PSK accepted for key "
818 "management, but no PSK configured.");
819 errors++;
820 }
821
822 if (errors) {
823 free(ssid);
824 ssid = NULL;
825 }
it's not necessary to define errors, or you need to check other previous
return value.
#16.
847 if (ssid == NULL)
848 errors ++;
849
850 config->ssid = ssid;
851
852 if (errors) {
853 wpa_config_free(config);
854 config = NULL;
855 }
856
857 return (config);
it's not necessary to define errors.
#17.
958 case 'D':
959 debug_mode = 1;
960 break;
-D option is not in usage output;
BR,
-Cecilia
Quaker Fang :
> Wireless WPA Supplicant (PSARC/2006/046) is now ready for code review.
> This work consists of a new wpa SMF service, along with WPA-related
> changes to dladm(1M), libdladm, and the mac_wifi, ath, and net80211
> kernel modules.
>
> This project consists of almost 10,000 changed or new lines of code.
> To make the review more manageable, we are splitting it into two parts.
>
> This is the first part, and consists of everything but[1] the net80211
> kernel module, ath driver and mac_wifi plugin changes.
> The second part will be sent out for review later.
>
> webrev: http://cr.grommit.com/~zf162725/cr_0308/
> workspace & cscope: /net/greatwall.prc/workspace/wifi-wpa-cr/usr/src
>
> Please provide feedback to this list. The review timer is set for two
> weeks (03/22/2007).
>
> Several Sun employees have graciously agreed to do the review
> (assignments are listed below), but we welcome comments from the
> community at-large.
>
> For part 1, there are 38 files to be reviewed, split into six areas:
>
> 1. WPA protocol: 3 files, ~3100 lines new
> wpa.c, wpa_supplicant, wpa_impl.h
>
> 2. DLPI related: 3 files, ~260 lines new
> l2_packet.[ch], ethernet.h
>
> 3. Misc: 6 files, ~1200 lines new
> eloop.[ch], wpa_enc.[ch], driver.h, driver_wifi.c
>
> 4. Packaging/Build: 17 files, ~600 lines changed
> Makefile, wpa.xml, svc-wpa, pkgdefs.
>
> 5. GLDv3 Userland: 7 files, ~600 lines changed
> libwladm.[ch], mapfile-vers, Makefile.com, secobj.c,
> libdladm.h, dladm.c
>
> 6. GLDv3 Kernel: 2 files, ~10 lines changed
> dld_drv.c, dld.h
>
> In the webrev, each file has been placed into one of the above areas.
> My proposed assignments are:
>
> Darren J Moffat and Cecilia Hu: 1. WPA Protocol
> Sagun Shakya: 2. DLPI related
> David Chieu: 3. Misc
> David Bustos or a team member of SMF team: 4. Packaging/Build, and
> libwladm.c (smf)
> Peter Memishian, Eric Cheng, Cathy Zhou: 5. GLDv3 Userland
> Peter Memishian: 6. GLDv3 Kernel
>
> *Thanks* for your assistance, and let us know if you have questions.
> Have fun! :-)
>
> -----
>
> [1] To provide more context when reviewing other kernel files, the
> net80211, ath, mac-wifi plugin files have been left in webrev/cscope.
>
> [2] In particular, there are some small changes between 2006/046 and
> the provided materials; we will return to the ARC after code review.
>
--
Cecilia.Hu at Sun.COM
Solaris x86 Engineering, Sun Microsystems
+86-10-82618200 ext. 82947 / 62673947
