| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <tencent_FACCCAB91E1C6F141FE1CA4AC1FF2420DA08@qq.com>
Date: Tue, 27 Sep 2022 20:55:08 +0800
From: "Xiaoke Wang" <xkernel.wang@...mail.com>
To: "Dan Carpenter" <dan.carpenter@...cle.com>
Cc: "Larry.Finger" <Larry.Finger@...inger.net>,
"florian.c.schilhabel"
<florian.c.schilhabel@...glemail.com>,
"gregkh" <gregkh@...uxfoundation.org>,
"linux-staging"
<linux-staging@...ts.linux.dev>,
"linux-kernel" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] staging: rtl8712: fix potential memory leak in _r8712_init_xmit_priv()
On Tue, Sep 27, 2022 03:57 PM, dan.carpenter@...cle.com wrote:
> On Mon, Sep 26, 2022 at 03:06:05PM +0800, xkernel.wang@...mail.com 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
Powered by blists - more mailing lists