* [PATCH v2 02/63] net/mlx5e: Avoid field-overflowing memcpy()
[not found] <20210818060533.3569517-1-keescook@chromium.org>
@ 2021-08-18 6:04 ` Kees Cook
2021-08-18 6:04 ` [PATCH v2 21/63] net/mlx5e: Use struct_group() for memcpy() region Kees Cook
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2021-08-18 6:04 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Saeed Mahameed, Leon Romanovsky, David S. Miller,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Jesper Dangaard Brouer, John Fastabend, netdev, linux-rdma, bpf,
Gustavo A. R. Silva, Greg Kroah-Hartman, Andrew Morton,
linux-wireless, dri-devel, linux-staging, linux-block,
linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use flexible arrays instead of zero-element arrays (which look like they
are always overflowing) and split the cross-field memcpy() into two halves
that can be appropriately bounds-checked by the compiler.
We were doing:
#define ETH_HLEN 14
#define VLAN_HLEN 4
...
#define MLX5E_XDP_MIN_INLINE (ETH_HLEN + VLAN_HLEN)
...
struct mlx5e_tx_wqe *wqe = mlx5_wq_cyc_get_wqe(wq, pi);
...
struct mlx5_wqe_eth_seg *eseg = &wqe->eth;
struct mlx5_wqe_data_seg *dseg = wqe->data;
...
memcpy(eseg->inline_hdr.start, xdptxd->data, MLX5E_XDP_MIN_INLINE);
target is wqe->eth.inline_hdr.start (which the compiler sees as being
2 bytes in size), but copying 18, intending to write across start
(really vlan_tci, 2 bytes). The remaining 16 bytes get written into
wqe->data[0], covering byte_count (4 bytes), lkey (4 bytes), and addr
(8 bytes).
struct mlx5e_tx_wqe {
struct mlx5_wqe_ctrl_seg ctrl; /* 0 16 */
struct mlx5_wqe_eth_seg eth; /* 16 16 */
struct mlx5_wqe_data_seg data[]; /* 32 0 */
/* size: 32, cachelines: 1, members: 3 */
/* last cacheline: 32 bytes */
};
struct mlx5_wqe_eth_seg {
u8 swp_outer_l4_offset; /* 0 1 */
u8 swp_outer_l3_offset; /* 1 1 */
u8 swp_inner_l4_offset; /* 2 1 */
u8 swp_inner_l3_offset; /* 3 1 */
u8 cs_flags; /* 4 1 */
u8 swp_flags; /* 5 1 */
__be16 mss; /* 6 2 */
__be32 flow_table_metadata; /* 8 4 */
union {
struct {
__be16 sz; /* 12 2 */
u8 start[2]; /* 14 2 */
} inline_hdr; /* 12 4 */
struct {
__be16 type; /* 12 2 */
__be16 vlan_tci; /* 14 2 */
} insert; /* 12 4 */
__be32 trailer; /* 12 4 */
}; /* 12 4 */
/* size: 16, cachelines: 1, members: 9 */
/* last cacheline: 16 bytes */
};
struct mlx5_wqe_data_seg {
__be32 byte_count; /* 0 4 */
__be32 lkey; /* 4 4 */
__be64 addr; /* 8 8 */
/* size: 16, cachelines: 1, members: 3 */
/* last cacheline: 16 bytes */
};
So, split the memcpy() so the compiler can reason about the buffer
sizes.
"pahole" shows no size nor member offset changes to struct mlx5e_tx_wqe
nor struct mlx5e_umr_wqe. "objdump -d" shows no meaningful object
code changes (i.e. only source line number induced differences and
optimizations).
Cc: Saeed Mahameed <saeedm@nvidia.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jesper Dangaard Brouer <hawk@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-rdma@vger.kernel.org
Cc: bpf@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 4 ++--
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 +++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index 4f6897c1ea8d..8997476c20cc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -200,7 +200,7 @@ static inline int mlx5e_get_max_num_channels(struct mlx5_core_dev *mdev)
struct mlx5e_tx_wqe {
struct mlx5_wqe_ctrl_seg ctrl;
struct mlx5_wqe_eth_seg eth;
- struct mlx5_wqe_data_seg data[0];
+ struct mlx5_wqe_data_seg data[];
};
struct mlx5e_rx_wqe_ll {
@@ -216,7 +216,7 @@ struct mlx5e_umr_wqe {
struct mlx5_wqe_ctrl_seg ctrl;
struct mlx5_wqe_umr_ctrl_seg uctrl;
struct mlx5_mkey_seg mkc;
- struct mlx5_mtt inline_mtts[0];
+ struct mlx5_mtt inline_mtts[];
};
extern const char mlx5e_self_tests[][ETH_GSTRING_LEN];
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
index 2f0df5cc1a2d..efae2444c26f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
@@ -341,8 +341,10 @@ mlx5e_xmit_xdp_frame(struct mlx5e_xdpsq *sq, struct mlx5e_xmit_data *xdptxd,
/* copy the inline part if required */
if (sq->min_inline_mode != MLX5_INLINE_MODE_NONE) {
- memcpy(eseg->inline_hdr.start, xdptxd->data, MLX5E_XDP_MIN_INLINE);
+ memcpy(eseg->inline_hdr.start, xdptxd->data, sizeof(eseg->inline_hdr.start));
eseg->inline_hdr.sz = cpu_to_be16(MLX5E_XDP_MIN_INLINE);
+ memcpy(dseg, xdptxd->data + sizeof(eseg->inline_hdr.start),
+ MLX5E_XDP_MIN_INLINE - sizeof(eseg->inline_hdr.start));
dma_len -= MLX5E_XDP_MIN_INLINE;
dma_addr += MLX5E_XDP_MIN_INLINE;
dseg++;
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 21/63] net/mlx5e: Use struct_group() for memcpy() region
[not found] <20210818060533.3569517-1-keescook@chromium.org>
2021-08-18 6:04 ` [PATCH v2 02/63] net/mlx5e: Avoid field-overflowing memcpy() Kees Cook
@ 2021-08-18 6:04 ` Kees Cook
2021-08-18 6:05 ` [PATCH v2 46/63] iw_cxgb4: Use memset_startat() for cpl_t5_pass_accept_rpl Kees Cook
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2021-08-18 6:04 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Saeed Mahameed, Leon Romanovsky, David S. Miller,
Jakub Kicinski, netdev, linux-rdma, Gustavo A. R. Silva,
Greg Kroah-Hartman, Andrew Morton, linux-wireless, dri-devel,
linux-staging, linux-block, linux-kbuild, clang-built-linux,
Rasmus Villemoes, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct vlan_ethhdr around members h_dest and
h_source, so they can be referenced together. This will allow memcpy()
and sizeof() to more easily reason about sizes, improve readability,
and avoid future warnings about writing beyond the end of h_dest.
"pahole" shows no size nor member offset changes to struct vlan_ethhdr.
"objdump -d" shows no object code changes.
Cc: Saeed Mahameed <saeedm@nvidia.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
Cc: linux-rdma@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +-
include/linux/if_vlan.h | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index c63d78eda606..39942a952736 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -207,7 +207,7 @@ static inline void mlx5e_insert_vlan(void *start, struct sk_buff *skb, u16 ihs)
int cpy1_sz = 2 * ETH_ALEN;
int cpy2_sz = ihs - cpy1_sz;
- memcpy(vhdr, skb->data, cpy1_sz);
+ memcpy(&vhdr->addrs, skb->data, cpy1_sz);
vhdr->h_vlan_proto = skb->vlan_proto;
vhdr->h_vlan_TCI = cpu_to_be16(skb_vlan_tag_get(skb));
memcpy(&vhdr->h_vlan_encapsulated_proto, skb->data + cpy1_sz, cpy2_sz);
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 41a518336673..45aad461aa34 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -46,8 +46,10 @@ struct vlan_hdr {
* @h_vlan_encapsulated_proto: packet type ID or len
*/
struct vlan_ethhdr {
- unsigned char h_dest[ETH_ALEN];
- unsigned char h_source[ETH_ALEN];
+ struct_group(addrs,
+ unsigned char h_dest[ETH_ALEN];
+ unsigned char h_source[ETH_ALEN];
+ );
__be16 h_vlan_proto;
__be16 h_vlan_TCI;
__be16 h_vlan_encapsulated_proto;
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 46/63] iw_cxgb4: Use memset_startat() for cpl_t5_pass_accept_rpl
[not found] <20210818060533.3569517-1-keescook@chromium.org>
2021-08-18 6:04 ` [PATCH v2 02/63] net/mlx5e: Avoid field-overflowing memcpy() Kees Cook
2021-08-18 6:04 ` [PATCH v2 21/63] net/mlx5e: Use struct_group() for memcpy() region Kees Cook
@ 2021-08-18 6:05 ` Kees Cook
2021-08-18 6:05 ` [PATCH v2 48/63] IB/mthca: Use memset_startat() for clearing mpt_entry Kees Cook
2021-08-18 6:05 ` [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr Kees Cook
4 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2021-08-18 6:05 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Potnuri Bharat Teja, Doug Ledford, Jason Gunthorpe,
Raju Rangoju, David S. Miller, Jakub Kicinski, linux-rdma,
netdev, Gustavo A. R. Silva, Greg Kroah-Hartman, Andrew Morton,
linux-wireless, dri-devel, linux-staging, linux-block,
linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
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, since everything
appears to perform a roundup (including allocation), just change the
size of the struct itself and add a build-time check to validate the
expected size.
Cc: Potnuri Bharat Teja <bharat@chelsio.com>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Raju Rangoju <rajur@chelsio.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: linux-rdma@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/infiniband/hw/cxgb4/cm.c | 5 +++--
drivers/net/ethernet/chelsio/cxgb4/t4_msg.h | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index 291471d12197..6519ea8ebf23 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -2471,7 +2471,8 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff *skb,
skb_get(skb);
rpl = cplhdr(skb);
if (!is_t4(adapter_type)) {
- skb_trim(skb, roundup(sizeof(*rpl5), 16));
+ BUILD_BUG_ON(sizeof(*rpl5) != roundup(sizeof(*rpl5), 16));
+ skb_trim(skb, sizeof(*rpl5));
rpl5 = (void *)rpl;
INIT_TP_WR(rpl5, ep->hwtid);
} else {
@@ -2487,7 +2488,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff *skb,
opt2 |= CONG_CNTRL_V(CONG_ALG_TAHOE);
opt2 |= T5_ISS_F;
rpl5 = (void *)rpl;
- memset(&rpl5->iss, 0, roundup(sizeof(*rpl5)-sizeof(*rpl), 16));
+ memset_after(rpl5, 0, iss);
if (peer2peer)
isn += 4;
rpl5->iss = cpu_to_be32(isn);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
index fed5f93bf620..26433a62d7f0 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
@@ -497,7 +497,7 @@ struct cpl_t5_pass_accept_rpl {
__be32 opt2;
__be64 opt0;
__be32 iss;
- __be32 rsvd;
+ __be32 rsvd[3];
};
struct cpl_act_open_req {
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 48/63] IB/mthca: Use memset_startat() for clearing mpt_entry
[not found] <20210818060533.3569517-1-keescook@chromium.org>
` (2 preceding siblings ...)
2021-08-18 6:05 ` [PATCH v2 46/63] iw_cxgb4: Use memset_startat() for cpl_t5_pass_accept_rpl Kees Cook
@ 2021-08-18 6:05 ` Kees Cook
2021-08-18 6:05 ` [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr Kees Cook
4 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2021-08-18 6:05 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Doug Ledford, Jason Gunthorpe, Max Gurtovoy,
linux-rdma, Gustavo A. R. Silva, Greg Kroah-Hartman,
Andrew Morton, linux-wireless, netdev, dri-devel, linux-staging,
linux-block, linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
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.
Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Max Gurtovoy <maxg@mellanox.com>
Cc: linux-rdma@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/infiniband/hw/mthca/mthca_mr.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/infiniband/hw/mthca/mthca_mr.c b/drivers/infiniband/hw/mthca/mthca_mr.c
index ce0e0867e488..1208e92ca3d3 100644
--- a/drivers/infiniband/hw/mthca/mthca_mr.c
+++ b/drivers/infiniband/hw/mthca/mthca_mr.c
@@ -469,8 +469,7 @@ int mthca_mr_alloc(struct mthca_dev *dev, u32 pd, int buffer_size_shift,
mpt_entry->start = cpu_to_be64(iova);
mpt_entry->length = cpu_to_be64(total_size);
- memset(&mpt_entry->lkey, 0,
- sizeof *mpt_entry - offsetof(struct mthca_mpt_entry, lkey));
+ memset_startat(mpt_entry, 0, lkey);
if (mr->mtt)
mpt_entry->mtt_seg =
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
[not found] <20210818060533.3569517-1-keescook@chromium.org>
` (3 preceding siblings ...)
2021-08-18 6:05 ` [PATCH v2 48/63] IB/mthca: Use memset_startat() for clearing mpt_entry Kees Cook
@ 2021-08-18 6:05 ` Kees Cook
2021-08-19 12:27 ` Jason Gunthorpe
4 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2021-08-18 6:05 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Leon Romanovsky, Doug Ledford, Jason Gunthorpe,
linux-rdma, Gustavo A. R. Silva, Greg Kroah-Hartman,
Andrew Morton, linux-wireless, netdev, dri-devel, linux-staging,
linux-block, linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct mlx5_ib_mr that should be
initialized to zero.
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-rdma@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index bf20a388eabe..f63bf204a7a1 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -644,6 +644,7 @@ struct mlx5_ib_mr {
struct ib_umem *umem;
/* This is zero'd when the MR is allocated */
+ struct_group(cleared,
union {
/* Used only while the MR is in the cache */
struct {
@@ -691,12 +692,13 @@ struct mlx5_ib_mr {
bool is_odp_implicit;
};
};
+ );
};
/* Zero the fields in the mr that are variant depending on usage */
static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
{
- memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
+ memset(&mr->cleared, 0, sizeof(mr->cleared));
}
static inline bool is_odp_mr(struct mlx5_ib_mr *mr)
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
2021-08-18 6:05 ` [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr Kees Cook
@ 2021-08-19 12:27 ` Jason Gunthorpe
2021-08-19 16:19 ` Kees Cook
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2021-08-19 12:27 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Leon Romanovsky, Doug Ledford, linux-rdma,
Gustavo A. R. Silva, Greg Kroah-Hartman, Andrew Morton,
linux-wireless, netdev, dri-devel, linux-staging, linux-block,
linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
On Tue, Aug 17, 2021 at 11:05:26PM -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.
>
> Add struct_group() to mark region of struct mlx5_ib_mr that should be
> initialized to zero.
>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: linux-rdma@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index bf20a388eabe..f63bf204a7a1 100644
> --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -644,6 +644,7 @@ struct mlx5_ib_mr {
> struct ib_umem *umem;
>
> /* This is zero'd when the MR is allocated */
> + struct_group(cleared,
> union {
> /* Used only while the MR is in the cache */
> struct {
> @@ -691,12 +692,13 @@ struct mlx5_ib_mr {
> bool is_odp_implicit;
> };
> };
> + );
> };
>
> /* Zero the fields in the mr that are variant depending on usage */
> static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> {
> - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> + memset(&mr->cleared, 0, sizeof(mr->cleared));
> }
Why not use the memset_after(mr->umem) here?
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
2021-08-19 12:27 ` Jason Gunthorpe
@ 2021-08-19 16:19 ` Kees Cook
2021-08-19 16:47 ` Jason Gunthorpe
0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2021-08-19 16:19 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, Leon Romanovsky, Doug Ledford, linux-rdma,
Gustavo A. R. Silva, Greg Kroah-Hartman, Andrew Morton,
linux-wireless, netdev, dri-devel, linux-staging, linux-block,
linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
On Thu, Aug 19, 2021 at 09:27:16AM -0300, Jason Gunthorpe wrote:
> On Tue, Aug 17, 2021 at 11:05:26PM -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.
> >
> > Add struct_group() to mark region of struct mlx5_ib_mr that should be
> > initialized to zero.
> >
> > Cc: Leon Romanovsky <leon@kernel.org>
> > Cc: Doug Ledford <dledford@redhat.com>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Cc: linux-rdma@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > index bf20a388eabe..f63bf204a7a1 100644
> > --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > @@ -644,6 +644,7 @@ struct mlx5_ib_mr {
> > struct ib_umem *umem;
> >
> > /* This is zero'd when the MR is allocated */
> > + struct_group(cleared,
> > union {
> > /* Used only while the MR is in the cache */
> > struct {
> > @@ -691,12 +692,13 @@ struct mlx5_ib_mr {
> > bool is_odp_implicit;
> > };
> > };
> > + );
> > };
> >
> > /* Zero the fields in the mr that are variant depending on usage */
> > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> > {
> > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> > + memset(&mr->cleared, 0, sizeof(mr->cleared));
> > }
>
> Why not use the memset_after(mr->umem) here?
I can certainly do that instead. In this series I've tended to opt
for groupings so the position of future struct member additions are
explicitly chosen. (i.e. reducing the chance that a zeroing of the new
member be a surprise.)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
2021-08-19 16:19 ` Kees Cook
@ 2021-08-19 16:47 ` Jason Gunthorpe
2021-08-19 18:14 ` Kees Cook
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2021-08-19 16:47 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Leon Romanovsky, Doug Ledford, linux-rdma,
Gustavo A. R. Silva, Greg Kroah-Hartman, Andrew Morton,
linux-wireless, netdev, dri-devel, linux-staging, linux-block,
linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
On Thu, Aug 19, 2021 at 09:19:08AM -0700, Kees Cook wrote:
> On Thu, Aug 19, 2021 at 09:27:16AM -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 17, 2021 at 11:05:26PM -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.
> > >
> > > Add struct_group() to mark region of struct mlx5_ib_mr that should be
> > > initialized to zero.
> > >
> > > Cc: Leon Romanovsky <leon@kernel.org>
> > > Cc: Doug Ledford <dledford@redhat.com>
> > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > Cc: linux-rdma@vger.kernel.org
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > index bf20a388eabe..f63bf204a7a1 100644
> > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > @@ -644,6 +644,7 @@ struct mlx5_ib_mr {
> > > struct ib_umem *umem;
> > >
> > > /* This is zero'd when the MR is allocated */
> > > + struct_group(cleared,
> > > union {
> > > /* Used only while the MR is in the cache */
> > > struct {
> > > @@ -691,12 +692,13 @@ struct mlx5_ib_mr {
> > > bool is_odp_implicit;
> > > };
> > > };
> > > + );
> > > };
> > >
> > > /* Zero the fields in the mr that are variant depending on usage */
> > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> > > {
> > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> > > + memset(&mr->cleared, 0, sizeof(mr->cleared));
> > > }
> >
> > Why not use the memset_after(mr->umem) here?
>
> I can certainly do that instead. In this series I've tended to opt
> for groupings so the position of future struct member additions are
> explicitly chosen. (i.e. reducing the chance that a zeroing of the new
> member be a surprise.)
I saw the earlier RDMA patches where using other memset techniques
though? Were there flex arrays or something that made groups infeasible?
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
2021-08-19 16:47 ` Jason Gunthorpe
@ 2021-08-19 18:14 ` Kees Cook
2021-08-20 12:34 ` Jason Gunthorpe
0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2021-08-19 18:14 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, Leon Romanovsky, Doug Ledford, linux-rdma,
Gustavo A. R. Silva, Greg Kroah-Hartman, Andrew Morton,
linux-wireless, netdev, dri-devel, linux-staging, linux-block,
linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
On Thu, Aug 19, 2021 at 01:47:57PM -0300, Jason Gunthorpe wrote:
> On Thu, Aug 19, 2021 at 09:19:08AM -0700, Kees Cook wrote:
> > On Thu, Aug 19, 2021 at 09:27:16AM -0300, Jason Gunthorpe wrote:
> > > On Tue, Aug 17, 2021 at 11:05:26PM -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.
> > > >
> > > > Add struct_group() to mark region of struct mlx5_ib_mr that should be
> > > > initialized to zero.
> > > >
> > > > Cc: Leon Romanovsky <leon@kernel.org>
> > > > Cc: Doug Ledford <dledford@redhat.com>
> > > > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > > > Cc: linux-rdma@vger.kernel.org
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > > index bf20a388eabe..f63bf204a7a1 100644
> > > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > > @@ -644,6 +644,7 @@ struct mlx5_ib_mr {
> > > > struct ib_umem *umem;
> > > >
> > > > /* This is zero'd when the MR is allocated */
> > > > + struct_group(cleared,
> > > > union {
> > > > /* Used only while the MR is in the cache */
> > > > struct {
> > > > @@ -691,12 +692,13 @@ struct mlx5_ib_mr {
> > > > bool is_odp_implicit;
> > > > };
> > > > };
> > > > + );
> > > > };
> > > >
> > > > /* Zero the fields in the mr that are variant depending on usage */
> > > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> > > > {
> > > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> > > > + memset(&mr->cleared, 0, sizeof(mr->cleared));
> > > > }
> > >
> > > Why not use the memset_after(mr->umem) here?
> >
> > I can certainly do that instead. In this series I've tended to opt
> > for groupings so the position of future struct member additions are
> > explicitly chosen. (i.e. reducing the chance that a zeroing of the new
> > member be a surprise.)
>
> I saw the earlier RDMA patches where using other memset techniques
> though? Were there flex arrays or something that made groups infeasible?
Which do you mean? When doing the conversions I tended to opt for
struct_group() since it provides more robust "intentionality". Strictly
speaking, the new memset helpers are doing field-spanning writes, but the
"clear to the end" pattern was so common it made sense to add the helpers,
as they're a bit less disruptive. It's totally up to you! :)
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
2021-08-19 18:14 ` Kees Cook
@ 2021-08-20 12:34 ` Jason Gunthorpe
2021-08-20 15:56 ` Kees Cook
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2021-08-20 12:34 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Leon Romanovsky, Doug Ledford, linux-rdma,
Gustavo A. R. Silva, Greg Kroah-Hartman, Andrew Morton,
linux-wireless, netdev, dri-devel, linux-staging, linux-block,
linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
On Thu, Aug 19, 2021 at 11:14:37AM -0700, Kees Cook wrote:
> Which do you mean? When doing the conversions I tended to opt for
> struct_group() since it provides more robust "intentionality". Strictly
> speaking, the new memset helpers are doing field-spanning writes, but the
> "clear to the end" pattern was so common it made sense to add the helpers,
> as they're a bit less disruptive. It's totally up to you! :)
Well, of the patches you cc'd to me only this one used the struct
group..
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
2021-08-20 12:34 ` Jason Gunthorpe
@ 2021-08-20 15:56 ` Kees Cook
0 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2021-08-20 15:56 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, Leon Romanovsky, Doug Ledford, linux-rdma,
Gustavo A. R. Silva, Greg Kroah-Hartman, Andrew Morton,
linux-wireless, netdev, dri-devel, linux-staging, linux-block,
linux-kbuild, clang-built-linux, Rasmus Villemoes,
linux-hardening
On Fri, Aug 20, 2021 at 09:34:00AM -0300, Jason Gunthorpe wrote:
> On Thu, Aug 19, 2021 at 11:14:37AM -0700, Kees Cook wrote:
>
> > Which do you mean? When doing the conversions I tended to opt for
> > struct_group() since it provides more robust "intentionality". Strictly
> > speaking, the new memset helpers are doing field-spanning writes, but the
> > "clear to the end" pattern was so common it made sense to add the helpers,
> > as they're a bit less disruptive. It's totally up to you! :)
>
> Well, of the patches you cc'd to me only this one used the struct
> group..
Understood. I've adjusted this for v3. Thanks!
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread