linux-kernel - Re: Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in _rtw_init_xmit

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20220331092013.GC12805@kadam>
Date:   Thu, 31 Mar 2022 12:20:13 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Xiaoke Wang <xkernel.wang@...mail.com>
Cc:     "larry.finger" <Larry.Finger@...inger.net>,
        phil <phil@...lpotter.co.uk>,
        gregkh <gregkh@...uxfoundation.org>,
        linux-staging <linux-staging@...ts.linux.dev>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in
 _rtw_init_xmit_priv()

On Thu, Mar 31, 2022 at 04:33:37PM +0800, Xiaoke Wang wrote:
> 
> On Thu, 31 Mar 2022 16:21:43 +0800, xkernel.wang@...mail.com wrote:
> >In fact, this is considering that we do not know where is the failure
> >from. In rtw_os_xmit_resource_alloc(), the failure can from 
> >
> >> pxmitbuf->pallocated_buf = kzalloc(alloc_sz, GFP_KERNEL);
> >
> > , but also can from 
> >
> >> 		pxmitbuf->pxmit_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> >
> > So if we do not handle the current failed item and just skip it, then some
> > memory may be ignored.
> 
> 
> 
> Maybe this should be the problem of rtw_os_xmit_resource_alloc() as it
> does properly handle the error from it.
> Anyway, I will attempt to fix them.
> 

It would be a lot easier to fix this code, if it were cleaner.

It goes to a lot of work to ensure that the pointers are aligned at the
4 byte mark, but memory from kmalloc() or vmalloc() is already aligned
at 8 bytes so there is no need.

	pxmitpriv->pallocated_frame_buf = vzalloc(NR_XMITFRAME * sizeof(struct xmit_frame) + 4);

The + 4 is for alignment.

	pxmitpriv->pxmit_frame_buf = (u8 *)N_BYTE_ALIGMENT((size_t)(pxmitpriv->pallocated_frame_buf), 4);

So then it stores pxmitpriv->pxmit_frame_buf which is the aligned
version of the pointer and the ->pallocated_frame_buf pointer which it
uses to free the memory.  Delete pxmitpriv->pallocated_frame_buf.  Just
allocate it directly.  Use vcalloc().

	pxmitpriv->pxmit_frame_buf = vzalloc(NR_XMITFRAME * sizeof(struct xmit_frame);

That stuff can all be done in one patch because it is:

Patch 1: Remove unnecessary alignment hacks.

This code goes to great lengths to ensure that the buffers are 4 bytes
aligned.  However that is not necessary as the vmalloc() and kmalloc()
functions return memory that is already aligned at 8 bytes.
1) No need for the temporary pxmitpriv->pallocated_frame_buf pointer to
   store unaligned memory.
2) No need to allocate "+ 4" extra bytes for alignment.
3) No need to call N_BYTE_ALIGMENT().

Patch 2: Use vcalloc() instead of vzalloc()
Patch 3: Declare the pxmitpriv->pxmit_frame_buf pointer as an array of
structs instead of an array of u8.  This allows us to remove some
casting.

Once the code is cleaner then the error handling will also be easier.

regards,
dan carpenter