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

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a6ltxooqc5kvgnw5xhinbt2k4g4l3o6osdvlp3mfcxdp6oo6oc@6cpny4hyexqj>
Date:   Wed, 10 May 2023 23:38:12 +0800
From:   Ruihan Li <lrh2000@....edu.cn>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     linux-mm@...ck.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Pasha Tatashin <pasha.tatashin@...een.com>,
        David Hildenbrand <david@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christoph Hellwig <hch@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        syzbot+fcf1a817ceb50935ce99@...kaller.appspotmail.comm,
        stable@...r.kernel.org, Ruihan Li <lrh2000@....edu.cn>
Subject: Re: [PATCH 1/4] usb: usbfs: Enforce page requirements for mmap

Hi Alan,

On Wed, May 10, 2023 at 10:37:45AM -0400, Alan Stern wrote:
> 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@...kaller.appspotmail.comm
> > Closes: https://lore.kernel.org/lkml/000000000000258e5e05fae79fc1@google.com/ [1]
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Ruihan Li <lrh2000@....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

I'm just a bit shocked to find out that different subsystems might
prefer different styles of coding. In the net subsystem, checkpatch.pl
will complain that:
	CHECK: Alignment should match open parenthesis

Nevertheless, in the next version, I'll follow the coding style that you
have pointed out.

Thanks,
Ruihan Li