On 8/23/2016 12:00 PM, Shiraz Saleem wrote:
> On Tue, Aug 23, 2016 at 04:41:35AM +0300, Leon Romanovsky wrote:
>> On Mon, Aug 22, 2016 at 07:01:47PM -0500, Shiraz Saleem wrote:
>>> From: Mustafa Ismail <mustafa.ismail@xxxxxxxxx>
>>>
>>> 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.
>
>>>
>>> Fixes: 4e9042e647ff ("i40iw: add hw and utils files")
>>>
>>> Reported-by: Stefan Assmann <sassmann@xxxxxxxxxx>
>>> Signed-off-by: Mustafa Ismail <mustafa.ismail@xxxxxxxxx>
>>> Signed-off-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
>>> ---
>>>
>>> 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.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG Key ID: 0E572FDD
Attachment:
signature.asc
Description: OpenPGP digital signature
