Re: [PATCH] mac80211: improved ieee80211_verify_alignment

Re: [PATCH] mac80211: improved ieee80211_verify_alignment

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

On Sat, 2009-11-28 at 15:47 +0100, Benoit Papillault wrote:
> From: Benoit PAPILLAULT <benoit.papillault@free.fr>
> 
> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] mac80211: improved ieee80211_verify_alignment

[Benoit PAPILLAULT]

Johannes Berg a écrit :
> On Sat, 2009-11-28 at 15:47 +0100, Benoit Papillault wrote:
>> From: Benoit PAPILLAULT <benoit.papillault@free.fr>
>>
>> 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