| Message ID | YD4enB/Er/5PWgUz@mwanda (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ipw2x00: potential buffer overflow in libipw_wx_set_encodeext() | expand |
On Tue, 2 Mar 2021 at 15:16, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > The "ext->key_len" is a u16 that comes from the user. If it's over > SCM_KEY_LEN (32) that could lead to memory corruption. > > Fixes: e0d369d1d969 ("[PATCH] ieee82011: Added WE-18 support to default wireless extension handler") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/net/wireless/intel/ipw2x00/libipw_wx.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > Cc: stable@vger.kernel.org Acked-by: Stanislav Yakovlev <stas.yakovlev@gmail.com> Thanks, and sorry for the long review. Stanislav. > diff --git a/drivers/net/wireless/intel/ipw2x00/libipw_wx.c b/drivers/net/wireless/intel/ipw2x00/libipw_wx.c > index a0cf78c418ac..27f15fa40528 100644 > --- a/drivers/net/wireless/intel/ipw2x00/libipw_wx.c > +++ b/drivers/net/wireless/intel/ipw2x00/libipw_wx.c > @@ -633,8 +633,10 @@ int libipw_wx_set_encodeext(struct libipw_device *ieee, > } > > if (ext->alg != IW_ENCODE_ALG_NONE) { > - memcpy(sec.keys[idx], ext->key, ext->key_len); > - sec.key_sizes[idx] = ext->key_len; > + int len = min_t(int, ext->key_len, SCM_KEY_LEN); > + > + memcpy(sec.keys[idx], ext->key, len); > + sec.key_sizes[idx] = len; > sec.flags |= (1 << idx); > if (ext->alg == IW_ENCODE_ALG_WEP) { > sec.encode_alg[idx] = SEC_ALG_WEP; > -- > 2.30.1 >
Stanislav Yakovlev <stas.yakovlev@gmail.com> writes: > On Tue, 2 Mar 2021 at 15:16, Dan Carpenter <dan.carpenter@oracle.com> wrote: >> >> The "ext->key_len" is a u16 that comes from the user. If it's over >> SCM_KEY_LEN (32) that could lead to memory corruption. >> >> Fixes: e0d369d1d969 ("[PATCH] ieee82011: Added WE-18 support to >> default wireless extension handler") >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> --- >> drivers/net/wireless/intel/ipw2x00/libipw_wx.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) [...] >> --- a/drivers/net/wireless/intel/ipw2x00/libipw_wx.c >> +++ b/drivers/net/wireless/intel/ipw2x00/libipw_wx.c >> @@ -633,8 +633,10 @@ int libipw_wx_set_encodeext(struct libipw_device *ieee, >> } >> >> if (ext->alg != IW_ENCODE_ALG_NONE) { >> - memcpy(sec.keys[idx], ext->key, ext->key_len); >> - sec.key_sizes[idx] = ext->key_len; >> + int len = min_t(int, ext->key_len, SCM_KEY_LEN); >> + >> + memcpy(sec.keys[idx], ext->key, len); >> + sec.key_sizes[idx] = len; >> sec.flags |= (1 << idx); >> if (ext->alg == IW_ENCODE_ALG_WEP) { >> sec.encode_alg[idx] = SEC_ALG_WEP; In another thread Linus gave a good tip about clamp_val(), I think it should be used it here as well to make the check safer and more readable. And also elsewhere in wireless code with similar limits.
diff --git a/drivers/net/wireless/intel/ipw2x00/libipw_wx.c b/drivers/net/wireless/intel/ipw2x00/libipw_wx.c index a0cf78c418ac..27f15fa40528 100644 --- a/drivers/net/wireless/intel/ipw2x00/libipw_wx.c +++ b/drivers/net/wireless/intel/ipw2x00/libipw_wx.c @@ -633,8 +633,10 @@ int libipw_wx_set_encodeext(struct libipw_device *ieee, } if (ext->alg != IW_ENCODE_ALG_NONE) { - memcpy(sec.keys[idx], ext->key, ext->key_len); - sec.key_sizes[idx] = ext->key_len; + int len = min_t(int, ext->key_len, SCM_KEY_LEN); + + memcpy(sec.keys[idx], ext->key, len); + sec.key_sizes[idx] = len; sec.flags |= (1 << idx); if (ext->alg == IW_ENCODE_ALG_WEP) { sec.encode_alg[idx] = SEC_ALG_WEP;
The "ext->key_len" is a u16 that comes from the user. If it's over SCM_KEY_LEN (32) that could lead to memory corruption. Fixes: e0d369d1d969 ("[PATCH] ieee82011: Added WE-18 support to default wireless extension handler") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/net/wireless/intel/ipw2x00/libipw_wx.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)