| Message ID | 20210818060533.3569517-46-keescook@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
Kees Cook <keescook@chromium.org> writes: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memset(), avoid intentionally writing across > neighboring fields. > > Use memset_startat() 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. Additionally split up a later > field-spanning memset() so that memset() can reason about the size. > > Cc: Kalle Valo <kvalo@codeaurora.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: ath11k@lists.infradead.org > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> To avoid conflicts I prefer taking this via my ath tree. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kees Cook <keescook@chromium.org> writes: > On Thu, Aug 19, 2021 at 04:19:37PM +0300, Kalle Valo wrote: >> Kees Cook <keescook@chromium.org> writes: >> >> > In preparation for FORTIFY_SOURCE performing compile-time and run-time >> > field bounds checking for memset(), avoid intentionally writing across >> > neighboring fields. >> > >> > Use memset_startat() 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. Additionally split up a later >> > field-spanning memset() so that memset() can reason about the size. >> > >> > Cc: Kalle Valo <kvalo@codeaurora.org> >> > Cc: "David S. Miller" <davem@davemloft.net> >> > Cc: Jakub Kicinski <kuba@kernel.org> >> > Cc: ath11k@lists.infradead.org >> > Cc: linux-wireless@vger.kernel.org >> > Cc: netdev@vger.kernel.org >> > Signed-off-by: Kees Cook <keescook@chromium.org> >> >> To avoid conflicts I prefer taking this via my ath tree. > > The memset helpers are introduced as part of this series, so that makes > things more difficult. Do you want me to create a branch with the > helpers that you can merge? Is this patch really worth the extra complexity? Why can't I apply this ath11k patch after the helpers have landed Linus' tree? That would be very simple.
On Sat, Aug 21, 2021 at 01:17:36PM +0300, Kalle Valo wrote: > Kees Cook <keescook@chromium.org> writes: > > > On Thu, Aug 19, 2021 at 04:19:37PM +0300, Kalle Valo wrote: > >> Kees Cook <keescook@chromium.org> writes: > >> > >> > In preparation for FORTIFY_SOURCE performing compile-time and run-time > >> > field bounds checking for memset(), avoid intentionally writing across > >> > neighboring fields. > >> > > >> > Use memset_startat() 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. Additionally split up a later > >> > field-spanning memset() so that memset() can reason about the size. > >> > > >> > Cc: Kalle Valo <kvalo@codeaurora.org> > >> > Cc: "David S. Miller" <davem@davemloft.net> > >> > Cc: Jakub Kicinski <kuba@kernel.org> > >> > Cc: ath11k@lists.infradead.org > >> > Cc: linux-wireless@vger.kernel.org > >> > Cc: netdev@vger.kernel.org > >> > Signed-off-by: Kees Cook <keescook@chromium.org> > >> > >> To avoid conflicts I prefer taking this via my ath tree. > > > > The memset helpers are introduced as part of this series, so that makes > > things more difficult. Do you want me to create a branch with the > > helpers that you can merge? > > Is this patch really worth the extra complexity? Why can't I apply this > ath11k patch after the helpers have landed Linus' tree? That would be > very simple. Not singularly, no. But I have a bit of a catch-22 in that I can't turn on greater FORTIFY strictness without first fixing the false positives, and I can't fix the false positives in "other" trees without those trees first having the helpers that get introduced by the FORTIFY series. :) Anyway, since we're close to the merge window anyway, the FORTIFY series won't land in 1 release at this point regardless, so I'll just get the helpers landed and we can do the individual pieces once the merge window closes. Wheee :)
diff --git a/drivers/net/wireless/ath/ath11k/hal_rx.c b/drivers/net/wireless/ath/ath11k/hal_rx.c index 325055ca41ab..0bab425f5dc9 100644 --- a/drivers/net/wireless/ath/ath11k/hal_rx.c +++ b/drivers/net/wireless/ath/ath11k/hal_rx.c @@ -29,8 +29,7 @@ static int ath11k_hal_reo_cmd_queue_stats(struct hal_tlv_hdr *tlv, FIELD_PREP(HAL_TLV_HDR_LEN, sizeof(*desc)); desc = (struct hal_reo_get_queue_stats *)tlv->value; - memset(&desc->queue_addr_lo, 0, - (sizeof(*desc) - sizeof(struct hal_reo_cmd_hdr))); + memset_startat(desc, 0, queue_addr_lo); desc->cmd.info0 &= ~HAL_REO_CMD_HDR_INFO0_STATUS_REQUIRED; if (cmd->flag & HAL_REO_CMD_FLG_NEED_STATUS) @@ -62,8 +61,7 @@ static int ath11k_hal_reo_cmd_flush_cache(struct ath11k_hal *hal, struct hal_tlv FIELD_PREP(HAL_TLV_HDR_LEN, sizeof(*desc)); desc = (struct hal_reo_flush_cache *)tlv->value; - memset(&desc->cache_addr_lo, 0, - (sizeof(*desc) - sizeof(struct hal_reo_cmd_hdr))); + memset_startat(desc, 0, cache_addr_lo); desc->cmd.info0 &= ~HAL_REO_CMD_HDR_INFO0_STATUS_REQUIRED; if (cmd->flag & HAL_REO_CMD_FLG_NEED_STATUS) @@ -101,8 +99,7 @@ static int ath11k_hal_reo_cmd_update_rx_queue(struct hal_tlv_hdr *tlv, FIELD_PREP(HAL_TLV_HDR_LEN, sizeof(*desc)); desc = (struct hal_reo_update_rx_queue *)tlv->value; - memset(&desc->queue_addr_lo, 0, - (sizeof(*desc) - sizeof(struct hal_reo_cmd_hdr))); + memset_startat(desc, 0, queue_addr_lo); desc->cmd.info0 &= ~HAL_REO_CMD_HDR_INFO0_STATUS_REQUIRED; if (cmd->flag & HAL_REO_CMD_FLG_NEED_STATUS) @@ -762,15 +759,17 @@ void ath11k_hal_reo_qdesc_setup(void *vaddr, int tid, u32 ba_window_size, * size changes and also send WMI message to FW to change the REO * queue descriptor in Rx peer entry as part of dp_rx_tid_update. */ - memset(ext_desc, 0, 3 * sizeof(*ext_desc)); + memset(ext_desc, 0, sizeof(*ext_desc)); ath11k_hal_reo_set_desc_hdr(&ext_desc->desc_hdr, HAL_DESC_REO_OWNED, HAL_DESC_REO_QUEUE_EXT_DESC, REO_QUEUE_DESC_MAGIC_DEBUG_PATTERN_1); ext_desc++; + memset(ext_desc, 0, sizeof(*ext_desc)); ath11k_hal_reo_set_desc_hdr(&ext_desc->desc_hdr, HAL_DESC_REO_OWNED, HAL_DESC_REO_QUEUE_EXT_DESC, REO_QUEUE_DESC_MAGIC_DEBUG_PATTERN_2); ext_desc++; + memset(ext_desc, 0, sizeof(*ext_desc)); ath11k_hal_reo_set_desc_hdr(&ext_desc->desc_hdr, HAL_DESC_REO_OWNED, HAL_DESC_REO_QUEUE_EXT_DESC, REO_QUEUE_DESC_MAGIC_DEBUG_PATTERN_3);
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memset(), avoid intentionally writing across neighboring fields. Use memset_startat() 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. Additionally split up a later field-spanning memset() so that memset() can reason about the size. Cc: Kalle Valo <kvalo@codeaurora.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: ath11k@lists.infradead.org Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/net/wireless/ath/ath11k/hal_rx.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)