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

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]

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.

> 
> 
> -- 
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: 0E572FDD
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]