Hi Andy, Sorry, I really hate doing this, but I found yet another problem :/ Hi Andy, Sorry, I really hate having comments again and again but never really thought about this earlier, the FCS removal thing you added made me think... > + * @max_length: total length we can parse into (eg, whole packet length) > + /* sanity check for allowed length and radiotap length field */ > + if (max_length < le16_to_cpu(radiotap_header->it_len)) > + return -EINVAL; > + iterator->max_length = le16_to_cpu(radiotap_header->it_len); This is fine, at first sight, but if you let the caller modify the skb like mac80211 now does with stripping the FCS, the max length really needs to be passed to each invocation of ieee80211_radiotap_iterator_next in order to catch invalid skbs. Mind you, we wouldn't Oops since trimming just moves the skb tail pointer, but something that indicated a longer length and then just have a packet like 0x00, 0x00, // <-- radiotap version 0x08, 0x00, // <- radiotap header length 0x10, 0x00, 0x00, 0x00, // <-- bitmap, FCS bit set which might not do the right thing and it'd be better IMHO to catch it explicitly. Also related to FCS, if you respin I think I'd like to have an explicit "0x00" entry in rt_sizes for it so it's obvious that it's intentionally 0, otherwise I'll post a patch after the code goes in. Another question: since there's no alignment requirement for the skb that contains the radiotap header, I think you need something like iterator->bitmap_shifter = le32_to_cpu(get_unaligned(iterator->next_bitmap)) instead of > + iterator->bitmap_shifter = > + le32_to_cpu(*iterator->next_bitmap); in many places. johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
