linux-kernel - Re: [PATCH] firmware: gsmi: Drop the use of dma_pool

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: <20201021085227.GA1102039@kroah.com>
Date:   Wed, 21 Oct 2020 10:52:27 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Furquan Shaikh <furquan@...gle.com>
Cc:     Ard Biesheuvel <ardb@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Prashant Malani <pmalani@...omium.org>,
        Arthur Heymans <arthur@...ymans.xyz>,
        Patrick Rudolph <patrick.rudolph@...ements.com>,
        Duncan Laurie <dlaurie@...gle.com>
Subject: Re: [PATCH] firmware: gsmi: Drop the use of dma_pool_* API functions

On Wed, Oct 21, 2020 at 12:37:52AM -0700, Furquan Shaikh wrote:
> On Tue, Oct 20, 2020 at 11:37 PM Ard Biesheuvel <ardb@...nel.org> wrote:
> >
> > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> > <gregkh@...uxfoundation.org> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 10:01:41PM -0700, Furquan Shaikh wrote:
> > > > GSMI driver uses dma_pool_* API functions for buffer allocation
> > > > because it requires that the SMI buffers are allocated within 32-bit
> > > > physical address space. However, this does not work well with IOMMU
> > > > since there is no real device and hence no domain associated with the
> > > > device.
> > > >
> > > > Since this is not a real device, it does not require any device
> > > > address(IOVA) for the buffer allocations. The only requirement is to
> > > > ensure that the physical address allocated to the buffer is within
> > > > 32-bit physical address space. This change allocates a page using
> > > > `get_zeroed_page()` and passes in GFP_DMA32 flag to ensure that the
> > > > page allocation is done in the DMA32 zone. All the buffer allocation
> > > > requests for gsmi_buf are then satisfed using this pre-allocated page
> > > > for the device.
> > >
> > > Are you sure that "GFP_DMA32" really does what you think it does?  A
> > > "normal" call with GFP_KERNEL" will give you memory that is properly
> > > dma-able.
> > >
> > > We should not be adding new GFP_DMA* users in the kernel in these days,
> > > just call dma_alloc*() and you should be fine.
> > >
> >
> > The point seems to be that this is not about DMA at all, and so there
> > is no device associated with the DMA allocation.
> >
> > The other 'master' is the CPU running firmware in an execution mode
> > where it can only access the bottom 4 GB of memory, and GFP_DMA32
> > happens to allocate from a zone which is guaranteed to be accessible
> > to the firmware.
> 
> Ard captured the context and requirement perfectly. GFP_DMA32
> satisfies the requirement for allocating memory from a zone which is
> accessible to the firmware in SMI mode. This seems to be one of the
> common ways  how other drivers and common code in the kernel currently
> allocate physical memory below the 4G boundary. Hence, I used the same
> mechanism in GSMI driver.

Then can you please document this a bit better in the changelog,
explaining why this is ok to use this obsolete api, and also in the code
itself so that no one tries to clean it up in the future?

thanks,

greg k-h