I am posting this to kernelnewbies because I do not know the correct
way to broach the issue elsewhere.
Code audit in drivers/staging on calls to strncpy().
Tree: gregKH/staging
commit d5adbfcd5f7bcc6fa58a41c5c5ada0e5c826ce2c
File: drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c
Function: int ieee80211_wx_set_essid(struct ieee80211_device *ieee,
struct iw_request_info *a,
union iwreq_data *wrqu, char *extra)
Lines: 415 - 429
spin_lock_irqsave(&ieee->lock, flags);
if (wrqu->essid.flags && wrqu->essid.length) {
/* first flush current network.ssid */
len = ((wrqu->essid.length-1) < IW_ESSID_MAX_SIZE) ? (wrqu->essid.length-1) : IW_ESSID_MAX_SIZE;
strncpy(ieee->current_network.ssid, extra, len+1);
ieee->current_network.ssid_len = len+1;
ieee->ssid_set = 1;
} else {
ieee->ssid_set = 0;
ieee->current_network.ssid[0] = '\0';
ieee->current_network.ssid_len = 0;
}
spin_unlock_irqrestore(&ieee->lock, flags);
Control can reach this snippet in two states:
1. wrqu->essid.length *can* be larger than IW_ESSID_MAX_SIZE
2. wrqu->essid.length *can not* be larger than IW_ESSID_MAX_SIZE (likely)
If wrqu->essid.length *can* be larger than IW_ESSID_MAX_SIZE:
- For the case when wrqu->essid.length > IW_ESSID_MAX_SIZE code sets
len to IW_ESSID_MAX_SIZE. If then, source string for call to strncpy()
(extra) is >= len+1 (IW_ESSID_MAX_SIZE+1) this results in a
non null terminated buffer.
If wrqu->essid.length *can not* be larger than IW_ESSID_MAX_SIZE:
- Setting of 'len' is overly complicated
- The code could be simplified by getting rid of the +1's and -1's
- The string would be null terminated without additional code (in line
with usage throughout the file).
There may not be an issue here, the code may be correct. It seems
overly complicated to prove that there is not an off by one error or a
buffer overflow vulnerability. The code does not seem to conform to
the rest of the file.
*If* there is a problem adding a null terminating byte would solve it
for both possible states.
Comments/questions
------------------
1. How would a more experienced kernel developer approach an issue
like this? With an initial patch and the discussion in a cover letter?
2. General kernel question:
Using compiler constants to define buffer storage in two differing
manners (i.e constant includes space for null byte vs constant does not
include space for null byte) especially when done using the *same*
constant seems to be asking for trouble.
I'm sure this has been discussed before at length, is there a
consensus on this? Is there a per subsystem consensus?
thanks,
Tobin.
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@xxxxxxxxxxxxxxxxx
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
