Re: [PATCH V2] i40iw: Do not set self-referencing pointer to NULL after kfree

On 8/23/2016 3:00 PM, Shiraz Saleem wrote:

On Tue, Aug 23, 2016 at 09:46:23PM +0300, Leon Romanovsky wrote:

quoted

On Tue, Aug 23, 2016 at 12:51:10PM -0400, Doug Ledford wrote:

quoted

On 8/23/2016 12:00 PM, Shiraz Saleem wrote:

quoted

On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:

quoted

On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:

quoted

From: Mustafa Ismail <redacted>

In i40iw_free_virt_mem(), do not set mem->va to NULL
after freeing it as mem->va is a self-referencing pointer
to mem.

Sorry, I failed to understand your commit message and your change.
What did you mean? How do you suppose to use mem->va pointer
after kfree() call on that pointer? Won't you have use-after-free bug in
such case?

The pointer mem->va cannot be used after kfree. But setting it to 
NULL would be writing to freed memory. In i40iw_puda_alloc_buf(), 
when a buffer is allocated of type struct i40iw_puda_buf, the address 
of the buffer itself is stored within the structure (in member buf_mem). 
When this pointer is freed, the structure containing the pointer is freed,
so writing to the structure would be writing to freed memory, which is 
what this fix is for.
 

quoted

quoted

Fixes: 4e9042e647ff ("i40iw: add hw and utils files")

Reported-by: Stefan Assmann <redacted>
Signed-off-by: Mustafa Ismail <redacted>
Signed-off-by: Shiraz Saleem <redacted>
---

V2: Fix typo in subject line.

 drivers/infiniband/hw/i40iw/i40iw_utils.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/hw/i40iw/i40iw_utils.c b/drivers/infiniband/hw/i40iw/i40iw_utils.c
index 0e8db0a..d5f5de2 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_utils.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_utils.c

@@ -674,7 +674,6 @@ enum i40iw_status_code i40iw_free_virt_mem(struct i40iw_hw *hw,
 	if (!mem)
 		return I40IW_ERR_PARAM;
 	kfree(mem->va);
-	mem->va = NULL;
 	return 0;

Your commit message is a bit hard to follow, but if I follow the
conversation, kfree(mem->va) is the same as kfree(mem), is it not?  If
it is, couldn't you just kfree(mem)?  That would avoid the confusion
here.  Also, if it matters, you can use a temporary pointer, aka:

p = mem->va;
mem->va = NULL;
kfree(p);

but, again, if mem->va is just a self-referencing pointer back to mem,
then why not just kfree(mem)?  I'm concerned that this convoluted way of
doing things will come back to haunt us later when people think they are
submitting a fix and simply reintroduce the same bug again.

Yeah, I totally agree with you.

OK, I see the the possible confusion of the commit message. 

Maybe the following is clearer:
"In i40iw_free_virt_mem(), do not set mem->va to NULL after freeing it, as mem->va is a pointer 
to the structure containing mem"

So kfree(mem->va) is not the same as kfree(mem). 

That makes more sense.  I still think things are a bit convoluted here,
and maybe even a code comment is in order.  I'll probably change it up a
little bit as I pull it in.


-- 
Doug Ledford [off-list ref]
    GPG Key ID: 0E572FDD