| Message ID | 20230218185701.never.779-kees@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | IB/rdmavt: Fix target union member for rvt_post_one_wr() | expand |
On 2/18/23 1:57 PM, Kees Cook wrote: > The "cplen" result used by the memcpy() into struct rvt_swqe "wqe" may > be sized to 80 for struct rvt_ud_wr (which is member "ud_wr", not "wr" > which is only 40 bytes in size). Change the destination union member so > the compiler can use the correct bounds check. > > struct rvt_swqe { > union { > struct ib_send_wr wr; /* don't use wr.sg_list */ > struct rvt_ud_wr ud_wr; > ... > }; > ... > }; > > Silences false positive memcpy() run-time warning: > > memcpy: detected field-spanning write (size 80) of single field "&wqe->wr" at drivers/infiniband/sw/rdmavt/qp.c:2043 (size 40) > > Reported-by: Zhang Yi <yizhan@redhat.com> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216561 > Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com> > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Cc: Leon Romanovsky <leon@kernel.org> > Cc: linux-rdma@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/infiniband/sw/rdmavt/qp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c > index 3acab569fbb9..3f707e1fa517 100644 > --- a/drivers/infiniband/sw/rdmavt/qp.c > +++ b/drivers/infiniband/sw/rdmavt/qp.c > @@ -2040,7 +2040,7 @@ static int rvt_post_one_wr(struct rvt_qp *qp, > wqe = rvt_get_swqe_ptr(qp, qp->s_head); > > /* cplen has length from above */ > - memcpy(&wqe->wr, wr, cplen); > + memcpy(&wqe->ud_wr, wr, cplen); > > wqe->length = 0; > j = 0; Thanks for the patch. We've been debating this issue internally since last week. The problem I have is this makes it look like everything is a "UD" req when it could be a different QP type. Maybe we just need a comment explaining. -Denny
On 2/21/23 8:00 AM, Dennis Dalessandro wrote: > On 2/18/23 1:57 PM, Kees Cook wrote: >> The "cplen" result used by the memcpy() into struct rvt_swqe "wqe" may >> be sized to 80 for struct rvt_ud_wr (which is member "ud_wr", not "wr" >> which is only 40 bytes in size). Change the destination union member so >> the compiler can use the correct bounds check. >> >> struct rvt_swqe { >> union { >> struct ib_send_wr wr; /* don't use wr.sg_list */ >> struct rvt_ud_wr ud_wr; >> ... >> }; >> ... >> }; >> >> Silences false positive memcpy() run-time warning: >> >> memcpy: detected field-spanning write (size 80) of single field "&wqe->wr" at drivers/infiniband/sw/rdmavt/qp.c:2043 (size 40) >> >> Reported-by: Zhang Yi <yizhan@redhat.com> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216561 >> Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com> >> Cc: Jason Gunthorpe <jgg@ziepe.ca> >> Cc: Leon Romanovsky <leon@kernel.org> >> Cc: linux-rdma@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> drivers/infiniband/sw/rdmavt/qp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c >> index 3acab569fbb9..3f707e1fa517 100644 >> --- a/drivers/infiniband/sw/rdmavt/qp.c >> +++ b/drivers/infiniband/sw/rdmavt/qp.c >> @@ -2040,7 +2040,7 @@ static int rvt_post_one_wr(struct rvt_qp *qp, >> wqe = rvt_get_swqe_ptr(qp, qp->s_head); >> >> /* cplen has length from above */ >> - memcpy(&wqe->wr, wr, cplen); >> + memcpy(&wqe->ud_wr, wr, cplen); >> >> wqe->length = 0; >> j = 0; > > Thanks for the patch. We've been debating this issue internally since last week. > The problem I have is this makes it look like everything is a "UD" req when it > could be a different QP type. Maybe we just need a comment explaining. For what it's worth I did test this patch. If you could add the comment that'd be great. If not I can touch it up later. Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
On Sat, 18 Feb 2023 10:57:05 -0800, Kees Cook wrote: > The "cplen" result used by the memcpy() into struct rvt_swqe "wqe" may > be sized to 80 for struct rvt_ud_wr (which is member "ud_wr", not "wr" > which is only 40 bytes in size). Change the destination union member so > the compiler can use the correct bounds check. > > struct rvt_swqe { > union { > struct ib_send_wr wr; /* don't use wr.sg_list */ > struct rvt_ud_wr ud_wr; > ... > }; > ... > }; > > [...] Applied, thanks! [1/1] IB/rdmavt: Fix target union member for rvt_post_one_wr() https://git.kernel.org/rdma/rdma/c/f2f6e1661d38fb Best regards,
diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c index 3acab569fbb9..3f707e1fa517 100644 --- a/drivers/infiniband/sw/rdmavt/qp.c +++ b/drivers/infiniband/sw/rdmavt/qp.c @@ -2040,7 +2040,7 @@ static int rvt_post_one_wr(struct rvt_qp *qp, wqe = rvt_get_swqe_ptr(qp, qp->s_head); /* cplen has length from above */ - memcpy(&wqe->wr, wr, cplen); + memcpy(&wqe->ud_wr, wr, cplen); wqe->length = 0; j = 0;
The "cplen" result used by the memcpy() into struct rvt_swqe "wqe" may be sized to 80 for struct rvt_ud_wr (which is member "ud_wr", not "wr" which is only 40 bytes in size). Change the destination union member so the compiler can use the correct bounds check. struct rvt_swqe { union { struct ib_send_wr wr; /* don't use wr.sg_list */ struct rvt_ud_wr ud_wr; ... }; ... }; Silences false positive memcpy() run-time warning: memcpy: detected field-spanning write (size 80) of single field "&wqe->wr" at drivers/infiniband/sw/rdmavt/qp.c:2043 (size 40) Reported-by: Zhang Yi <yizhan@redhat.com> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216561 Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: Leon Romanovsky <leon@kernel.org> Cc: linux-rdma@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/infiniband/sw/rdmavt/qp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)