[PATCH for-rc] RDMA/efa: Clear the admin command buffer prior to its submission

[PATCH for-rc] RDMA/efa: Clear the admin command buffer prior to its submission

[Gal Pressman]

We cannot rely on the entry memcpy as we only copy the actual size of
the command, the rest of the bytes must be memset to zero.

Fixes: 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
Reviewed-by: Daniel Kranzdorf <dkkranzd@amazon.com>
Reviewed-by: Firas JahJah <firasj@amazon.com>
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
 drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/efa/efa_com.c b/drivers/infiniband/hw/efa/efa_com.c
index 3c412bc5b94f..0778f4f7dccd 100644
--- a/drivers/infiniband/hw/efa/efa_com.c
+++ b/drivers/infiniband/hw/efa/efa_com.c
@@ -317,6 +317,7 @@ static struct efa_comp_ctx *__efa_com_submit_admin_cmd(struct efa_com_admin_queu
 						       struct efa_admin_acq_entry *comp,
 						       size_t comp_size_in_bytes)
 {
+	struct efa_admin_aq_entry *aqe;
 	struct efa_comp_ctx *comp_ctx;
 	u16 queue_size_mask;
 	u16 cmd_id;
@@ -350,7 +351,9 @@ static struct efa_comp_ctx *__efa_com_submit_admin_cmd(struct efa_com_admin_queu
 
 	reinit_completion(&comp_ctx->wait_event);
 
-	memcpy(&aq->sq.entries[pi], cmd, cmd_size_in_bytes);
+	aqe = &aq->sq.entries[pi];
+	memset(aqe, 0, sizeof(*aqe));
+	memcpy(aqe, cmd, cmd_size_in_bytes);
 
 	aq->sq.pc++;
 	atomic64_inc(&aq->stats.submitted_cmd);
-- 
2.23.0

Re: [PATCH for-rc] RDMA/efa: Clear the admin command buffer prior to its submission

[Jason Gunthorpe]

On Tue, Nov 12, 2019 at 11:26:08AM +0200, Gal Pressman wrote:
> We cannot rely on the entry memcpy as we only copy the actual size of
> the command, the rest of the bytes must be memset to zero.
> 
> Fixes: 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
> Reviewed-by: Daniel Kranzdorf <dkkranzd@amazon.com>
> Reviewed-by: Firas JahJah <firasj@amazon.com>
> Signed-off-by: Gal Pressman <galpress@amazon.com>
> ---
>  drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

This is quite late in the -rc cycle for such a vauge description. What
is the user visible impact of passing non-zero memory beyond the
command length?

Jason

Re: [PATCH for-rc] RDMA/efa: Clear the admin command buffer prior to its submission

[Gal Pressman]

On 13/11/2019 2:17, Jason Gunthorpe wrote:
> On Tue, Nov 12, 2019 at 11:26:08AM +0200, Gal Pressman wrote:
>> We cannot rely on the entry memcpy as we only copy the actual size of
>> the command, the rest of the bytes must be memset to zero.
>>
>> Fixes: 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
>> Reviewed-by: Daniel Kranzdorf <dkkranzd@amazon.com>
>> Reviewed-by: Firas JahJah <firasj@amazon.com>
>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>> ---
>>  drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> This is quite late in the -rc cycle for such a vauge description. What
> is the user visible impact of passing non-zero memory beyond the
> command length?

Currently providing non-zero memory will not have any user visible impact.
However, since admin commands are extendable (in a backwards compatible way)
everything beyond the size of the command must be cleared to prevent issues in
the future.

Re: [PATCH for-rc] RDMA/efa: Clear the admin command buffer prior to its submission

[Jason Gunthorpe]

On Tue, Nov 12, 2019 at 11:26:08AM +0200, Gal Pressman wrote:
> We cannot rely on the entry memcpy as we only copy the actual size of
> the command, the rest of the bytes must be memset to zero.
> 
> Fixes: 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
> Reviewed-by: Daniel Kranzdorf <dkkranzd@amazon.com>
> Reviewed-by: Firas JahJah <firasj@amazon.com>
> Signed-off-by: Gal Pressman <galpress@amazon.com>
> ---
>  drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

This isn't really -rc material since the device will have to be
compatible with these old kernels for quite some time.

Applied to for-next with the better description

Thanks,
Jason

Re: [PATCH for-rc] RDMA/efa: Clear the admin command buffer prior to its submission

[Gal Pressman]

On 14/11/2019 17:59, Jason Gunthorpe wrote:
> On Tue, Nov 12, 2019 at 11:26:08AM +0200, Gal Pressman wrote:
>> We cannot rely on the entry memcpy as we only copy the actual size of
>> the command, the rest of the bytes must be memset to zero.
>>
>> Fixes: 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
>> Reviewed-by: Daniel Kranzdorf <dkkranzd@amazon.com>
>> Reviewed-by: Firas JahJah <firasj@amazon.com>
>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>> ---
>>  drivers/infiniband/hw/efa/efa_com.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> This isn't really -rc material since the device will have to be
> compatible with these old kernels for quite some time.

That's why I pushed it to -rc, this bug fix must be applied to these kernels
(5.2, 5.3) as well through stable.

If we're trying to avoid pushing this change late in the -rc cycle, I'm fine
with this patch going through -next and backported to 5.4 stable as well.