On Tue, Sep 27, 2022 03:57 PM, dan.carpenter@xxxxxxxxxx wrote: > On Mon, Sep 26, 2022 at 03:06:05PM +0800, xkernel.wang@xxxxxxxxxxx wrote: >> diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c >> index 090345b..dcf3f76 100644 >> --- a/drivers/staging/rtl8712/rtl871x_xmit.c >> +++ b/drivers/staging/rtl8712/rtl871x_xmit.c >> @@ -117,11 +117,8 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv, >> _init_queue(&pxmitpriv->pending_xmitbuf_queue); >> pxmitpriv->pallocated_xmitbuf = >> kmalloc(NR_XMITBUFF * sizeof(struct xmit_buf) + 4, GFP_ATOMIC); >> - if (!pxmitpriv->pallocated_xmitbuf) { >> -kfree(pxmitpriv->pallocated_frame_buf); >> -pxmitpriv->pallocated_frame_buf = NULL; >> -return -ENOMEM; >> - } >> + if (!pxmitpriv->pallocated_xmitbuf) >> +goto free_frame_buf; Note here: may have the same concern. >> pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 - >> ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3); >> pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf; >> @@ -130,12 +127,14 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv, >> pxmitbuf->pallocated_buf = >> kmalloc(MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ, GFP_ATOMIC); >> if (!pxmitbuf->pallocated_buf) >> - return -ENOMEM; >> + goto free_xmitbuf; >> pxmitbuf->pbuf = pxmitbuf->pallocated_buf + XMITBUF_ALIGN_SZ - >> ((addr_t) (pxmitbuf->pallocated_buf) & >> (XMITBUF_ALIGN_SZ - 1)); >> -if (r8712_xmit_resource_alloc(padapter, pxmitbuf)) >> - return -ENOMEM; >> +if (r8712_xmit_resource_alloc(padapter, pxmitbuf)) { >> + kfree(pxmitbuf->pallocated_buf); >> + goto free_xmitbuf; >> +} >> list_add_tail(&pxmitbuf->list, >> &(pxmitpriv->free_xmitbuf_queue.queue)); > > > pxmitbuf points to somewhere in the middle of pxmitpriv->pallocated_xmitbuf. > We add it to the list here. > >> pxmitbuf++; >> @@ -146,6 +145,18 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv, >> init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry); >> tasklet_setup(&pxmitpriv->xmit_tasklet, r8712_xmit_bh); >> return 0; >> + >> +free_xmitbuf: >> + pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf; >> + while (i--) { >> +r8712_xmit_resource_free(padapter, pxmitbuf); >> +kfree(pxmitbuf->pallocated_buf); >> +pxmitbuf++; >> + } >> + kfree(pxmitpriv->pallocated_xmitbuf); > > But then we free pxmitpriv->pallocated_xmitbuf here but it the memory > is still on the list. So that means there will be a use after free > eventually. Yes, the memory address is still on the list, and at the above position of `Note`, the address of `pxmitpriv->pallocated_frame_buf` is also left on a list named `pxmitpriv->free_xmit_queue`. However, these lists are in `pxmitpriv` and this function is for initializing `pxmitpriv`. When this function fails, the probe function of this driver will finally fail. So I guess the list in `pxmitpriv` will not be accessed. Please let me know if you still hold such concerns, I am glad to find a time (in 2 weeks I guess) to add `list_del_init()` on the error paths to clear all the improper pointing records. Regards, Xiaoke Wangÿÿÿ