Re: [PATCH 1/4] usb: usbfs: Enforce page requirements for mmap

[Alan Stern <stern@rowland.harvard.edu>]

On Wed, May 10, 2023 at 04:55:24PM +0800, Ruihan Li wrote:
> The current implementation of usbdev_mmap uses usb_alloc_coherent to
> allocate memory pages that will later be mapped into the user space.
> Meanwhile, usb_alloc_coherent employs three different methods to
> allocate memory, as outlined below:
>  * If hcd->localmem_pool is non-null, it uses gen_pool_dma_alloc to
>    allocate memory.
>  * If DMA is not available, it uses kmalloc to allocate memory.
>  * Otherwise, it uses dma_alloc_coherent.
> 
> However, it should be noted that gen_pool_dma_alloc does not guarantee
> that the resulting memory will be page-aligned. Furthermore, trying to
> map slab pages (i.e., memory allocated by kmalloc) into the user space
> is not resonable and can lead to problems, such as a type confusion bug
> when PAGE_TABLE_CHECK=y [1].
> 
> To address these issues, this patch introduces hcd_alloc_coherent_pages,
> which addresses the above two problems. Specifically,
> hcd_alloc_coherent_pages uses gen_pool_dma_alloc_align instead of
> gen_pool_dma_alloc to ensure that the memory is page-aligned. To replace
> kmalloc, hcd_alloc_coherent_pages directly allocates pages by calling
> __get_free_pages.
> 
> Reported-by: syzbot+fcf1a817ceb50935ce99@syzkaller.appspotmail.comm
> Closes: https://lore.kernel.org/lkml/000000000000258e5e05fae79fc1@google.com/ [1]
> Cc: stable@vger.kernel.org
> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> ---

I'm never quite sure about when it makes sense to complain about 
stylistic issues.  Nevertheless, I'm going to do so here...

>  drivers/usb/core/buffer.c | 41 +++++++++++++++++++++++++++++++++++++++
>  drivers/usb/core/devio.c  |  9 +++++----
>  include/linux/usb/hcd.h   |  5 +++++
>  3 files changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index fbb087b72..6010ef9f5 100644
> --- a/drivers/usb/core/buffer.c
> +++ b/drivers/usb/core/buffer.c
> @@ -172,3 +172,44 @@ void hcd_buffer_free(
>  	}
>  	dma_free_coherent(hcd->self.sysdev, size, addr, dma);
>  }
> +
> +void *hcd_buffer_alloc_pages(struct usb_hcd *hcd, size_t size,
> +			     gfp_t mem_flags, dma_addr_t *dma)
> +{
> +	if (size == 0)
> +		return NULL;
> +
> +	if (hcd->localmem_pool)
> +		return gen_pool_dma_alloc_align(hcd->localmem_pool,
> +						size, dma, PAGE_SIZE);

C isn't Lisp.  Expressions in C are not based entirely around 
parentheses, and it's not necessary to align our code based on the 
parenthesized sub-expressions to avoid hopelessly confusing the reader.

The style used in this file (and many other places in the USB core) is 
to indent continuation lines by two tab stops.  The same comment applies 
to all the other continuation lines you added or changed in this patch 
and in patch 2/4.

Alan Stern