On Wed, Apr 20, 2022 at 04:52:36PM +0200, Fabio M. De Francesco wrote: > On mercoledì 20 aprile 2022 14:23:28 CEST Rebecca Mckeever wrote: > > Currently, these three get_key functions return -1 when the provided len > > value is less a specific key length value, which can result in buffer > > overflow depending on how the returned value is used. These functions are > > used in three places in ieee80211/ieee80211_wx.c: > > > > ieee80211_wx_get_encode() : > > The behavior of this function will be unchanged. > > > > ieee80211_wx_get_encode_ext() : > > The result of the get_key function is written to ext->key_len, > > resulting in a buffer overflow if the result is negative. > > > > ieee80211_wx_set_encode() : > > The behavior of this function will change. When len is less than the > > key length value, it will set a default key of all 0. > > > > Suggested-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > Signed-off-by: Rebecca Mckeever <remckee0@xxxxxxxxx> > > --- > > drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 2 +- > > drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 2 +- > > drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > I was not able to find the message where Dan suggested this solution. > However it looks that we actually have problems in those callers when they > get '-1' as the return code. > > I didn't look at the code with much attention but I think that returning > '0' to signal errors is not so good. > > If I'm not missing something from the context, I'd rather return '-EINVAL' > and change the callers to check for this specific error that would signal > we have "len less than a specific len value". If so, act in accordance to > the returned -EINVAL. > > For example, in a caller you may test: > > if (ext->key_len == -EINVAL || <some other test, if required>) > <error path> > <success path> These are valid questions. Ideally the errors wouldn't happen. Maybe they don't happen? The error handling is copied from ieee80211_wx_get_encode_ext(). An all zero password (rot13 security) is better than plaintext. So this is a conservative fix which potentially fixes some memory corruption and doesn't break anything. A more extensive fix would require someone who understands the code better or can test it. regards, dan carpenter
