| Message ID | 20210616203744.1248551-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | IB/mlx4: Avoid field-overflowing memcpy() | expand |
On Wed, Jun 16, 2021 at 01:37:44PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring array fields. > > Use the ether_addr_copy() helper instead, as already done for smac. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/infiniband/hw/mlx4/qp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c > index 2ae22bf50016..4a2ef7daaded 100644 > --- a/drivers/infiniband/hw/mlx4/qp.c > +++ b/drivers/infiniband/hw/mlx4/qp.c > @@ -3144,7 +3144,7 @@ static int build_mlx_header(struct mlx4_ib_qp *qp, const struct ib_ud_wr *wr, > mlx->sched_prio = cpu_to_be16(pcp); > > ether_addr_copy(sqp->ud_header.eth.smac_h, ah->av.eth.s_mac); > - memcpy(sqp->ud_header.eth.dmac_h, ah->av.eth.mac, 6); > + ether_addr_copy(sqp->ud_header.eth.dmac_h, ah->av.eth.mac); > memcpy(&ctrl->srcrb_flags16[0], ah->av.eth.mac, 2); > memcpy(&ctrl->imm, ah->av.eth.mac + 2, 4); I don't understand the last three lines. We are copying 6 bytes to ah->av.eth.mac and immediately after that overwriting them. Jack, Do you remember what you wanted to achieve in commit 6ee51a4e866b ("mlx4: Adjust QP1 multiplexing for RoCE/SRIOV") Thanks > > -- > 2.25.1 >
On Thu, Jun 17, 2021 at 10:24:58AM +0300, Leon Romanovsky wrote: > On Wed, Jun 16, 2021 at 01:37:44PM -0700, Kees Cook wrote: > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memcpy(), memmove(), and memset(), avoid > > intentionally writing across neighboring array fields. > > > > Use the ether_addr_copy() helper instead, as already done for smac. > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > drivers/infiniband/hw/mlx4/qp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c > > index 2ae22bf50016..4a2ef7daaded 100644 > > --- a/drivers/infiniband/hw/mlx4/qp.c > > +++ b/drivers/infiniband/hw/mlx4/qp.c > > @@ -3144,7 +3144,7 @@ static int build_mlx_header(struct mlx4_ib_qp *qp, const struct ib_ud_wr *wr, > > mlx->sched_prio = cpu_to_be16(pcp); > > > > ether_addr_copy(sqp->ud_header.eth.smac_h, ah->av.eth.s_mac); > > - memcpy(sqp->ud_header.eth.dmac_h, ah->av.eth.mac, 6); > > + ether_addr_copy(sqp->ud_header.eth.dmac_h, ah->av.eth.mac); > > memcpy(&ctrl->srcrb_flags16[0], ah->av.eth.mac, 2); > > memcpy(&ctrl->imm, ah->av.eth.mac + 2, 4); > > I don't understand the last three lines. We are copying 6 bytes to > ah->av.eth.mac and immediately after that overwriting them. I'm not following (the memcpy() is replaced by ether_addr_copy()). I only see ah->av.eth.mac being read from (not written to). And the destinations are {s,d}mac_h: ah->av.eth.s_mac -> sqp->ud_header.eth.smac_h (s_mac to smac_h: 6 bytes) ah->av.eth.mac -> sqp->ud_header.eth.dmac_h (mac to dmac_h: 6 bytes) after that I see: ah->av.eth.mac -> &ctrl->srcrb_flags16[0] (2 bytes) ah->av.eth.mac + 2 -> ctrl->imm (4 bytes) The last two copies mac again in pieces, but I don't know what any of this is actually used for, which I what I assume you're asking about. :) > Jack, > > Do you remember what you wanted to achieve in commit > 6ee51a4e866b ("mlx4: Adjust QP1 multiplexing for RoCE/SRIOV") > > Thanks -Kees
On Thu, Jun 17, 2021 at 12:46:43PM -0700, Kees Cook wrote: > On Thu, Jun 17, 2021 at 10:24:58AM +0300, Leon Romanovsky wrote: > > On Wed, Jun 16, 2021 at 01:37:44PM -0700, Kees Cook wrote: > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > > field bounds checking for memcpy(), memmove(), and memset(), avoid > > > intentionally writing across neighboring array fields. > > > > > > Use the ether_addr_copy() helper instead, as already done for smac. > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > drivers/infiniband/hw/mlx4/qp.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c > > > index 2ae22bf50016..4a2ef7daaded 100644 > > > --- a/drivers/infiniband/hw/mlx4/qp.c > > > +++ b/drivers/infiniband/hw/mlx4/qp.c > > > @@ -3144,7 +3144,7 @@ static int build_mlx_header(struct mlx4_ib_qp *qp, const struct ib_ud_wr *wr, > > > mlx->sched_prio = cpu_to_be16(pcp); > > > > > > ether_addr_copy(sqp->ud_header.eth.smac_h, ah->av.eth.s_mac); > > > - memcpy(sqp->ud_header.eth.dmac_h, ah->av.eth.mac, 6); > > > + ether_addr_copy(sqp->ud_header.eth.dmac_h, ah->av.eth.mac); > > > memcpy(&ctrl->srcrb_flags16[0], ah->av.eth.mac, 2); > > > memcpy(&ctrl->imm, ah->av.eth.mac + 2, 4); > > > > I don't understand the last three lines. We are copying 6 bytes to > > ah->av.eth.mac and immediately after that overwriting them. > > I'm not following (the memcpy() is replaced by ether_addr_copy()). Forget it, it was me who mixed src with dst in the memcpy() signature. Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
On Wed, Jun 16, 2021 at 01:37:44PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring array fields. > > Use the ether_addr_copy() helper instead, as already done for smac. > > Signed-off-by: Kees Cook <keescook@chromium.org> > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> > --- > drivers/infiniband/hw/mlx4/qp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied to for-next, thanks Jason
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c index 2ae22bf50016..4a2ef7daaded 100644 --- a/drivers/infiniband/hw/mlx4/qp.c +++ b/drivers/infiniband/hw/mlx4/qp.c @@ -3144,7 +3144,7 @@ static int build_mlx_header(struct mlx4_ib_qp *qp, const struct ib_ud_wr *wr, mlx->sched_prio = cpu_to_be16(pcp); ether_addr_copy(sqp->ud_header.eth.smac_h, ah->av.eth.s_mac); - memcpy(sqp->ud_header.eth.dmac_h, ah->av.eth.mac, 6); + ether_addr_copy(sqp->ud_header.eth.dmac_h, ah->av.eth.mac); memcpy(&ctrl->srcrb_flags16[0], ah->av.eth.mac, 2); memcpy(&ctrl->imm, ah->av.eth.mac + 2, 4);
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring array fields. Use the ether_addr_copy() helper instead, as already done for smac. Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/infiniband/hw/mlx4/qp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)