| Message ID | 20210727205855.411487-40-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
On Tue, Jul 27, 2021 at 01:58:30PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memset(), avoid intentionally writing across > neighboring fields. > > Use memset_after() so memset() doesn't get confused about writing > beyond the destination member that is intended to be the starting point > of zeroing through the end of the struct. > > Note that the common helper, ieee80211_tx_info_clear_status(), does NOT > clear ack_signal, but the open-coded versions do. All three perform > checks that the ack_signal position hasn't changed, though. Quick ping on this question: there is a mismatch between the common helper and the other places that do this. Is there a bug here? > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > Should these each be clearing the same region? Because they're currently not. > --- > drivers/net/wireless/ath/carl9170/tx.c | 4 +--- > drivers/net/wireless/intersil/p54/txrx.c | 4 +--- > include/net/mac80211.h | 4 +--- > 3 files changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c > index 88444fe6d1c6..6d2115639434 100644 > --- a/drivers/net/wireless/ath/carl9170/tx.c > +++ b/drivers/net/wireless/ath/carl9170/tx.c > @@ -278,9 +278,7 @@ static void carl9170_tx_release(struct kref *ref) > BUILD_BUG_ON( > offsetof(struct ieee80211_tx_info, status.ack_signal) != 20); > > - memset(&txinfo->status.ack_signal, 0, > - sizeof(struct ieee80211_tx_info) - > - offsetof(struct ieee80211_tx_info, status.ack_signal)); > + memset_after(&txinfo->status, 0, rates); > > if (atomic_read(&ar->tx_total_queued)) > ar->tx_schedule = true; > diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c > index 873fea59894f..f71b355f8583 100644 > --- a/drivers/net/wireless/intersil/p54/txrx.c > +++ b/drivers/net/wireless/intersil/p54/txrx.c > @@ -431,9 +431,7 @@ static void p54_rx_frame_sent(struct p54_common *priv, struct sk_buff *skb) > * Clear manually, ieee80211_tx_info_clear_status would > * clear the counts too and we need them. > */ > - memset(&info->status.ack_signal, 0, > - sizeof(struct ieee80211_tx_info) - > - offsetof(struct ieee80211_tx_info, status.ack_signal)); > + memset_after(&info->status, 0, rates); > BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, > status.ack_signal) != 20); > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index d8a1d09a2141..7abc1427aa8c 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -1200,9 +1200,7 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info) > > BUILD_BUG_ON( > offsetof(struct ieee80211_tx_info, status.ack_signal) != 20); > - memset(&info->status.ampdu_ack_len, 0, > - sizeof(struct ieee80211_tx_info) - > - offsetof(struct ieee80211_tx_info, status.ampdu_ack_len)); > + memset_after(&info->status, 0, ack_signal); > } > > > -- > 2.30.2 >
On Sat, 2021-07-31 at 08:55 -0700, Kees Cook wrote: > On Tue, Jul 27, 2021 at 01:58:30PM -0700, Kees Cook wrote: > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memset(), avoid intentionally writing across > > neighboring fields. > > > > Use memset_after() so memset() doesn't get confused about writing > > beyond the destination member that is intended to be the starting point > > of zeroing through the end of the struct. > > > > Note that the common helper, ieee80211_tx_info_clear_status(), does NOT > > clear ack_signal, but the open-coded versions do. All three perform > > checks that the ack_signal position hasn't changed, though. > > Quick ping on this question: there is a mismatch between the common > helper and the other places that do this. Is there a bug here? Yes. The common helper should also clear ack_signal, but that was broken by commit e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES"), because that commit changed the order of the fields and updated carl9170 and p54 properly but not the common helper... It doesn't actually matter much because ack_signal is normally filled in afterwards, and even if it isn't, it's just for statistics. The correct thing to do here would be to memset_after(&info->status, 0, rates); johannes
On Sat, 2021-07-31 at 08:55 -0700, Kees Cook wrote: > > > @@ -278,9 +278,7 @@ static void carl9170_tx_release(struct kref *ref) > > BUILD_BUG_ON( > > offsetof(struct ieee80211_tx_info, status.ack_signal) != 20); > > > > > > - memset(&txinfo->status.ack_signal, 0, > > - sizeof(struct ieee80211_tx_info) - > > - offsetof(struct ieee80211_tx_info, status.ack_signal)); > > + memset_after(&txinfo->status, 0, rates); FWIW, I think we should also remove the BUILD_BUG_ON() now in all the places - that was meant to give people a hint to update if some field ordering etc. changed, but now that it's "after rates" this is no longer necessary. johannes
On Fri, Aug 13, 2021 at 09:40:07AM +0200, Johannes Berg wrote: > On Sat, 2021-07-31 at 08:55 -0700, Kees Cook wrote: > > On Tue, Jul 27, 2021 at 01:58:30PM -0700, Kees Cook wrote: > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > > field bounds checking for memset(), avoid intentionally writing across > > > neighboring fields. > > > > > > Use memset_after() so memset() doesn't get confused about writing > > > beyond the destination member that is intended to be the starting point > > > of zeroing through the end of the struct. > > > > > > Note that the common helper, ieee80211_tx_info_clear_status(), does NOT > > > clear ack_signal, but the open-coded versions do. All three perform > > > checks that the ack_signal position hasn't changed, though. > > > > Quick ping on this question: there is a mismatch between the common > > helper and the other places that do this. Is there a bug here? > > Yes. > > The common helper should also clear ack_signal, but that was broken by > commit e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES"), because > that commit changed the order of the fields and updated carl9170 and p54 > properly but not the common helper... It looks like p54 actually uses the rates, which is why it does this manually. I can't see why carl9170 does this manually, though. > It doesn't actually matter much because ack_signal is normally filled in > afterwards, and even if it isn't, it's just for statistics. > > The correct thing to do here would be to > > memset_after(&info->status, 0, rates); Sounds good; I will adjust these (and drop the BULID_BUG_ONs, as you suggest in the next email). Thanks! -Kees
On Fri, 2021-08-13 at 09:08 -0700, Kees Cook wrote: > > > > The common helper should also clear ack_signal, but that was broken by > > commit e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES"), because > > that commit changed the order of the fields and updated carl9170 and p54 > > properly but not the common helper... > > It looks like p54 actually uses the rates, which is why it does this > manually. I can't see why carl9170 does this manually, though. mac80211 also uses the rates later again on status reporting, it just expects the # of attempts to be filled etc. I haven't looked at carl9170, but I would expect it to do something there and do it correctly, even though old it's a well-written driver and uses mac80211 rate control, so this would need to be correct for decent performance. But I guess it could be that the helper could be used because the rates were already handed to the firmware, and the code was just copy/pasted from p54 (the drivers were, IIRC, developed by the same folks) johannes
diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c index 88444fe6d1c6..6d2115639434 100644 --- a/drivers/net/wireless/ath/carl9170/tx.c +++ b/drivers/net/wireless/ath/carl9170/tx.c @@ -278,9 +278,7 @@ static void carl9170_tx_release(struct kref *ref) BUILD_BUG_ON( offsetof(struct ieee80211_tx_info, status.ack_signal) != 20); - memset(&txinfo->status.ack_signal, 0, - sizeof(struct ieee80211_tx_info) - - offsetof(struct ieee80211_tx_info, status.ack_signal)); + memset_after(&txinfo->status, 0, rates); if (atomic_read(&ar->tx_total_queued)) ar->tx_schedule = true; diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c index 873fea59894f..f71b355f8583 100644 --- a/drivers/net/wireless/intersil/p54/txrx.c +++ b/drivers/net/wireless/intersil/p54/txrx.c @@ -431,9 +431,7 @@ static void p54_rx_frame_sent(struct p54_common *priv, struct sk_buff *skb) * Clear manually, ieee80211_tx_info_clear_status would * clear the counts too and we need them. */ - memset(&info->status.ack_signal, 0, - sizeof(struct ieee80211_tx_info) - - offsetof(struct ieee80211_tx_info, status.ack_signal)); + memset_after(&info->status, 0, rates); BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.ack_signal) != 20); diff --git a/include/net/mac80211.h b/include/net/mac80211.h index d8a1d09a2141..7abc1427aa8c 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1200,9 +1200,7 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info) BUILD_BUG_ON( offsetof(struct ieee80211_tx_info, status.ack_signal) != 20); - memset(&info->status.ampdu_ack_len, 0, - sizeof(struct ieee80211_tx_info) - - offsetof(struct ieee80211_tx_info, status.ampdu_ack_len)); + memset_after(&info->status, 0, ack_signal); }
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memset(), avoid intentionally writing across neighboring fields. Use memset_after() so memset() doesn't get confused about writing beyond the destination member that is intended to be the starting point of zeroing through the end of the struct. Note that the common helper, ieee80211_tx_info_clear_status(), does NOT clear ack_signal, but the open-coded versions do. All three perform checks that the ack_signal position hasn't changed, though. Signed-off-by: Kees Cook <keescook@chromium.org> --- Should these each be clearing the same region? Because they're currently not. --- drivers/net/wireless/ath/carl9170/tx.c | 4 +--- drivers/net/wireless/intersil/p54/txrx.c | 4 +--- include/net/mac80211.h | 4 +--- 3 files changed, 3 insertions(+), 9 deletions(-)