* [PATCH] mac80211: fix radiotap header generation
@ 2009-10-28 8:58 Johannes Berg
0 siblings, 0 replies; 4+ messages in thread
From: Johannes Berg @ 2009-10-28 8:58 UTC (permalink / raw)
To: John Linville; +Cc: linux-wireless, Bruno Randolf
In
commit 601ae7f25aea58f208a7f640f6174aac0652403a
Author: Bruno Randolf <br1@einfach.org>
Date: Thu May 8 19:22:43 2008 +0200
mac80211: make rx radiotap header more flexible
code was added that tried to align the radiotap header
position in memory based on the radiotap header length.
Quite obviously, that is completely useless.
Instead of trying to do that, use unaligned accesses
to generate the radiotap header. To properly do that,
we also need to mark struct ieee80211_radiotap_header
packed, but that is fine since it's already packed
(and it should be marked packed anyway since its a
wire format).
Cc: Bruno Randolf <br1@einfach.org>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
I'm not sure it should be sent to stable, it only affects platforms that
don't do unaligned accesses well.
include/net/ieee80211_radiotap.h | 2 +-
net/mac80211/rx.c | 26 ++++++++++++--------------
2 files changed, 13 insertions(+), 15 deletions(-)
--- wireless-testing.orig/net/mac80211/rx.c 2009-10-28 08:43:27.000000000 +0100
+++ wireless-testing/net/mac80211/rx.c 2009-10-28 08:58:24.000000000 +0100
@@ -95,10 +95,6 @@ ieee80211_rx_radiotap_len(struct ieee802
if (len & 1) /* padding for RX_FLAGS if necessary */
len++;
- /* make sure radiotap starts at a naturally aligned address */
- if (len % 8)
- len = roundup(len, 8);
-
return len;
}
@@ -116,6 +112,7 @@ ieee80211_add_rx_radiotap_header(struct
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
struct ieee80211_radiotap_header *rthdr;
unsigned char *pos;
+ u16 rx_flags = 0;
rthdr = (struct ieee80211_radiotap_header *)skb_push(skb, rtap_len);
memset(rthdr, 0, rtap_len);
@@ -134,7 +131,7 @@ ieee80211_add_rx_radiotap_header(struct
/* IEEE80211_RADIOTAP_TSFT */
if (status->flag & RX_FLAG_TSFT) {
- *(__le64 *)pos = cpu_to_le64(status->mactime);
+ put_unaligned_le64(status->mactime, pos);
rthdr->it_present |=
cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);
pos += 8;
@@ -166,17 +163,17 @@ ieee80211_add_rx_radiotap_header(struct
pos++;
/* IEEE80211_RADIOTAP_CHANNEL */
- *(__le16 *)pos = cpu_to_le16(status->freq);
+ put_unaligned_le16(status->freq, pos);
pos += 2;
if (status->band == IEEE80211_BAND_5GHZ)
- *(__le16 *)pos = cpu_to_le16(IEEE80211_CHAN_OFDM |
- IEEE80211_CHAN_5GHZ);
+ put_unaligned_le16(IEEE80211_CHAN_OFDM | IEEE80211_CHAN_5GHZ,
+ pos);
else if (rate->flags & IEEE80211_RATE_ERP_G)
- *(__le16 *)pos = cpu_to_le16(IEEE80211_CHAN_OFDM |
- IEEE80211_CHAN_2GHZ);
+ put_unaligned_le16(IEEE80211_CHAN_OFDM | IEEE80211_CHAN_2GHZ,
+ pos);
else
- *(__le16 *)pos = cpu_to_le16(IEEE80211_CHAN_CCK |
- IEEE80211_CHAN_2GHZ);
+ put_unaligned_le16(IEEE80211_CHAN_CCK | IEEE80211_CHAN_2GHZ,
+ pos);
pos += 2;
/* IEEE80211_RADIOTAP_DBM_ANTSIGNAL */
@@ -205,10 +202,11 @@ ieee80211_add_rx_radiotap_header(struct
/* IEEE80211_RADIOTAP_RX_FLAGS */
/* ensure 2 byte alignment for the 2 byte field as required */
- if ((pos - (unsigned char *)rthdr) & 1)
+ if ((pos - (u8 *)rthdr) & 1)
pos++;
if (status->flag & RX_FLAG_FAILED_PLCP_CRC)
- *(__le16 *)pos |= cpu_to_le16(IEEE80211_RADIOTAP_F_RX_BADPLCP);
+ rx_flags |= IEEE80211_RADIOTAP_F_RX_BADPLCP;
+ put_unaligned_le16(rx_flags, pos);
pos += 2;
}
--- wireless-testing.orig/include/net/ieee80211_radiotap.h 2009-10-28 08:46:35.000000000 +0100
+++ wireless-testing/include/net/ieee80211_radiotap.h 2009-10-28 08:51:49.000000000 +0100
@@ -80,7 +80,7 @@ struct ieee80211_radiotap_header {
* Additional extensions are made
* by setting bit 31.
*/
-};
+} __packed;
/* Name Data type Units
* ---- --------- -----
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] mac80211: fix radiotap header generation
@ 2021-11-09 9:02 Johannes Berg
2021-11-09 16:13 ` Sid Hayn
2021-11-09 17:17 ` Kees Cook
0 siblings, 2 replies; 4+ messages in thread
From: Johannes Berg @ 2021-11-09 9:02 UTC (permalink / raw)
To: linux-wireless; +Cc: Kees Cook, Johannes Berg, stable, Sid Hayn
From: Johannes Berg <johannes.berg@intel.com>
In commit 8c89f7b3d3f2 ("mac80211: Use flex-array for radiotap header
bitmap") we accidentally pointed the position to the wrong place, so
we overwrite a present bitmap, and thus cause all kinds of trouble.
To see the issue, note that the previous code read:
pos = (void *)(it_present + 1);
The requirement now is that we need to calculate pos via it_optional,
to not trigger the compiler hardening checks, as:
pos = (void *)&rthdr->it_optional[...];
Rewriting the original expression, we get (obviously, since that just
adds "+ x - x" terms):
pos = (void *)(it_present + 1 + rthdr->it_optional - rthdr->it_optional)
and moving the "+ rthdr->it_optional" outside to be used as an array:
pos = (void *)&rthdr->it_optional[it_present + 1 - rthdr->it_optional];
The original is off by one, fix it.
Cc: stable@vger.kernel.org
Fixes: 8c89f7b3d3f2 ("mac80211: Use flex-array for radiotap header bitmap")
Reported-by: Sid Hayn <sidhayn@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
net/mac80211/rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index fc5c608d02e2..3562730ea0f8 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -364,7 +364,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
* the compiler to think we have walked past the end of the
* struct member.
*/
- pos = (void *)&rthdr->it_optional[it_present - rthdr->it_optional];
+ pos = (void *)&rthdr->it_optional[it_present + 1 - rthdr->it_optional];
/* the order of the following fields is important */
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] mac80211: fix radiotap header generation
2021-11-09 9:02 Johannes Berg
@ 2021-11-09 16:13 ` Sid Hayn
2021-11-09 17:17 ` Kees Cook
1 sibling, 0 replies; 4+ messages in thread
From: Sid Hayn @ 2021-11-09 16:13 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Kees Cook, Johannes Berg, stable
Please add my tested-by as well. I tested with and without this patch
on multiple chipsets and everything appears functional now. Thanks
for the quick fix!
Tested-by: Sid Hayn <sidhayn@gmail.com>
On Tue, Nov 9, 2021 at 4:02 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>
>
> In commit 8c89f7b3d3f2 ("mac80211: Use flex-array for radiotap header
> bitmap") we accidentally pointed the position to the wrong place, so
> we overwrite a present bitmap, and thus cause all kinds of trouble.
>
> To see the issue, note that the previous code read:
>
> pos = (void *)(it_present + 1);
>
> The requirement now is that we need to calculate pos via it_optional,
> to not trigger the compiler hardening checks, as:
>
> pos = (void *)&rthdr->it_optional[...];
>
> Rewriting the original expression, we get (obviously, since that just
> adds "+ x - x" terms):
>
> pos = (void *)(it_present + 1 + rthdr->it_optional - rthdr->it_optional)
>
> and moving the "+ rthdr->it_optional" outside to be used as an array:
>
> pos = (void *)&rthdr->it_optional[it_present + 1 - rthdr->it_optional];
>
> The original is off by one, fix it.
>
> Cc: stable@vger.kernel.org
> Fixes: 8c89f7b3d3f2 ("mac80211: Use flex-array for radiotap header bitmap")
> Reported-by: Sid Hayn <sidhayn@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> net/mac80211/rx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index fc5c608d02e2..3562730ea0f8 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -364,7 +364,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
> * the compiler to think we have walked past the end of the
> * struct member.
> */
> - pos = (void *)&rthdr->it_optional[it_present - rthdr->it_optional];
> + pos = (void *)&rthdr->it_optional[it_present + 1 - rthdr->it_optional];
>
> /* the order of the following fields is important */
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mac80211: fix radiotap header generation
2021-11-09 9:02 Johannes Berg
2021-11-09 16:13 ` Sid Hayn
@ 2021-11-09 17:17 ` Kees Cook
1 sibling, 0 replies; 4+ messages in thread
From: Kees Cook @ 2021-11-09 17:17 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Johannes Berg, stable, Sid Hayn
On Tue, Nov 09, 2021 at 10:02:04AM +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> In commit 8c89f7b3d3f2 ("mac80211: Use flex-array for radiotap header
> bitmap") we accidentally pointed the position to the wrong place, so
> we overwrite a present bitmap, and thus cause all kinds of trouble.
>
> To see the issue, note that the previous code read:
>
> pos = (void *)(it_present + 1);
>
> The requirement now is that we need to calculate pos via it_optional,
> to not trigger the compiler hardening checks, as:
>
> pos = (void *)&rthdr->it_optional[...];
>
> Rewriting the original expression, we get (obviously, since that just
> adds "+ x - x" terms):
>
> pos = (void *)(it_present + 1 + rthdr->it_optional - rthdr->it_optional)
>
> and moving the "+ rthdr->it_optional" outside to be used as an array:
>
> pos = (void *)&rthdr->it_optional[it_present + 1 - rthdr->it_optional];
>
> The original is off by one, fix it.
>
> Cc: stable@vger.kernel.org
> Fixes: 8c89f7b3d3f2 ("mac80211: Use flex-array for radiotap header bitmap")
> Reported-by: Sid Hayn <sidhayn@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Argh! Thank you for the fix and sorry for the trouble the earlier patch
created!
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> net/mac80211/rx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index fc5c608d02e2..3562730ea0f8 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -364,7 +364,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
> * the compiler to think we have walked past the end of the
> * struct member.
> */
> - pos = (void *)&rthdr->it_optional[it_present - rthdr->it_optional];
> + pos = (void *)&rthdr->it_optional[it_present + 1 - rthdr->it_optional];
>
> /* the order of the following fields is important */
>
> --
> 2.31.1
>
--
Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-09 17:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-10-28 8:58 [PATCH] mac80211: fix radiotap header generation Johannes Berg
2021-11-09 9:02 Johannes Berg
2021-11-09 16:13 ` Sid Hayn
2021-11-09 17:17 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.