| Message ID | 20230815181603.f576bd983875.I1efbeef082c3f7094037882f213202d760848eb7@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211_hwsim: drop short frames | expand |
On 8/15/2023 9:16 AM, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Frames that don't even have addr1 are clearly not valid, > drop those early in the netlink/wmediumd path here. > > Reported-by: syzbot+b2645b5bf1512b81fa22@syzkaller.appspotmail.com > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > drivers/net/wireless/virtual/mac80211_hwsim.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c > index f446fd0e8cd0..8f1e4420ed1e 100644 > --- a/drivers/net/wireless/virtual/mac80211_hwsim.c > +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c > @@ -5626,14 +5626,15 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2, > frame_data_len = nla_len(info->attrs[HWSIM_ATTR_FRAME]); > frame_data = (void *)nla_data(info->attrs[HWSIM_ATTR_FRAME]); > > + if (frame_data_len < offsetofend(typeof(*hdr), addr1) || curious why addr1. if the frame ends after addr1 then don't you have a problem in mac80211_hwsim_rx() when it passes hdr->addr2 to ieee80211_find_sta_by_link_addrs()? > + frame_data_len > IEEE80211_MAX_DATA_LEN) > + goto err; > + > /* Allocate new skb here */ > skb = alloc_skb(frame_data_len, GFP_KERNEL); > if (skb == NULL) > goto err; > > - if (frame_data_len > IEEE80211_MAX_DATA_LEN) > - goto err; > - > /* Copy the data */ > skb_put_data(skb, frame_data, frame_data_len); >
On Tue, 2023-08-15 at 11:20 -0700, Jeff Johnson wrote: > On 8/15/2023 9:16 AM, Johannes Berg wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > > > Frames that don't even have addr1 are clearly not valid, > > drop those early in the netlink/wmediumd path here. > > > > Reported-by: syzbot+b2645b5bf1512b81fa22@syzkaller.appspotmail.com > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > > --- > > drivers/net/wireless/virtual/mac80211_hwsim.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c > > index f446fd0e8cd0..8f1e4420ed1e 100644 > > --- a/drivers/net/wireless/virtual/mac80211_hwsim.c > > +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c > > @@ -5626,14 +5626,15 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2, > > frame_data_len = nla_len(info->attrs[HWSIM_ATTR_FRAME]); > > frame_data = (void *)nla_data(info->attrs[HWSIM_ATTR_FRAME]); > > > > + if (frame_data_len < offsetofend(typeof(*hdr), addr1) || > > curious why addr1. > I figured that was the shortest legal 802.11 frame, e.g. an ACK frame. > if the frame ends after addr1 then don't you have a > problem in mac80211_hwsim_rx() when it passes hdr->addr2 to > ieee80211_find_sta_by_link_addrs()? But of course you're right about that, so we should just make sure we have at least the full struct ieee80211_hdr_3addr here, I guess. johannes
diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c index f446fd0e8cd0..8f1e4420ed1e 100644 --- a/drivers/net/wireless/virtual/mac80211_hwsim.c +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c @@ -5626,14 +5626,15 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2, frame_data_len = nla_len(info->attrs[HWSIM_ATTR_FRAME]); frame_data = (void *)nla_data(info->attrs[HWSIM_ATTR_FRAME]); + if (frame_data_len < offsetofend(typeof(*hdr), addr1) || + frame_data_len > IEEE80211_MAX_DATA_LEN) + goto err; + /* Allocate new skb here */ skb = alloc_skb(frame_data_len, GFP_KERNEL); if (skb == NULL) goto err; - if (frame_data_len > IEEE80211_MAX_DATA_LEN) - goto err; - /* Copy the data */ skb_put_data(skb, frame_data, frame_data_len);