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

Doug Ledford
2016-08-23T19:05:42Z

[-- Attachment #1.1: Type: text/plain, Size: 3636 bytes --]

On 8/23/2016 3:00 PM, Shiraz Saleem wrote:
> On Tue, Aug 23, 2016 at 09:46:23PM +0300, Leon Romanovsky wrote:
>> On Tue, Aug 23, 2016 at 12:51:10PM -0400, Doug Ledford wrote:
>>> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>>>
>>>>>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>>> Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>>> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>>> ---
>>>>>>
>>>>>> 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 <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG Key ID: 0E572FDD

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]