On Sat, 2009-11-28 at 15:47 +0100, Benoit Papillault wrote:
> From: Benoit PAPILLAULT <[email protected]>
>
> ieee80211_verify_alignment has been improved to avoid small 802.11 frame (<2
> bytes) and skip checking for data alignment when there is no 802.11 data (when
> the frame length is less or egal to the header length)
None of this is necessary.
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -386,10 +386,23 @@ static void ieee80211_verify_alignment(struct ieee80211_rx_data *rx)
> "unaligned packet at 0x%p\n", rx->skb->data))
> return;
>
> + /* before using the hdr->frame_control field, we need to check that
> + * skb contains at least 2 bytes */
> +
> + if (rx->skb->len < 2)
> + return ;
> +
Frames shorter than 16 bytes never reach this point.
> if (!ieee80211_is_data_present(hdr->frame_control))
> return;
>
> hdrlen = ieee80211_hdrlen(hdr->frame_control);
> +
> + /* before checking data alignment, we need to check that skb contains
> + * at least 1 byte of data */
> +
> + if (rx->skb->len <= hdrlen)
> + return;
> +
Even if this could happen it's not true -- we do not need a byte of data
to verify that it's aligned properly. After all, even the empty string
can be aligned -- we never actually dereference the data there.
johannes
Johannes Berg a écrit :
> On Sat, 2009-11-28 at 15:47 +0100, Benoit Papillault wrote:
>> From: Benoit PAPILLAULT <[email protected]>
>>
>> ieee80211_verify_alignment has been improved to avoid small 802.11 frame (<2
>> bytes) and skip checking for data alignment when there is no 802.11 data (when
>> the frame length is less or egal to the header length)
>
> None of this is necessary.
>
>> --- a/net/mac80211/rx.c
>> +++ b/net/mac80211/rx.c
>> @@ -386,10 +386,23 @@ static void ieee80211_verify_alignment(struct ieee80211_rx_data *rx)
>> "unaligned packet at 0x%p\n", rx->skb->data))
>> return;
>>
>> + /* before using the hdr->frame_control field, we need to check that
>> + * skb contains at least 2 bytes */
>> +
>> + if (rx->skb->len < 2)
>> + return ;
>> +
>
> Frames shorter than 16 bytes never reach this point.
>
>> if (!ieee80211_is_data_present(hdr->frame_control))
>> return;
>>
>> hdrlen = ieee80211_hdrlen(hdr->frame_control);
>> +
>> + /* before checking data alignment, we need to check that skb contains
>> + * at least 1 byte of data */
>> +
>> + if (rx->skb->len <= hdrlen)
>> + return;
>> +
>
> Even if this could happen it's not true -- we do not need a byte of data
> to verify that it's aligned properly. After all, even the empty string
> can be aligned -- we never actually dereference the data there.
>
> johannes
My purpose was to make ieee80211_verify_alignment autonomous, ie without
assuming that other functions have done some checks before. I got some
problems since this function is called by
__ieee80211_rx_handle_packet(), called by ieee80211_rx() where skb can
be anything (and is anything since i'm doing injection!).
Following our discussion, I understand that :
- for the first case, frames with skb->len < 16 are already filtered out
by ieee80211_rx_monitor() when it calls should_drop_frame().
- for the second case, we don't dereference data, so we are on the safe
side. I'll further study this point since my feeling was that I got some
warning where I would not expect them. The case would be : skb->len = 24
for instance, hdrlen = 26 and skb->data aligned on a 2 bytes boundary
only. The current code would issue a warning even if no payload part is
present.
So, forget this patch for now.
Regards,
Benoit