Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 9 Mar 2021 19:54:16 +0200
From:   Mike Rapoport <rppt@linux.ibm.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     George Kennedy <george.kennedy@oracle.com>,
        David Hildenbrand <david@redhat.com>,
        Robert Moore <robert.moore@intel.com>,
        Erik Kaneda <erik.kaneda@intel.com>,
        Rafael Wysocki <rafael.j.wysocki@intel.com>,
        Len Brown <lenb@kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        "open list:ACPI COMPONENT ARCHITECTURE (ACPICA)" <devel@acpica.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Dhaval Giani <dhaval.giani@oracle.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Oscar Salvador <osalvador@suse.de>,
        Wei Yang <richard.weiyang@linux.alibaba.com>,
        Pankaj Gupta <pankaj.gupta.linux@gmail.com>,
        Michal Hocko <mhocko@suse.com>
Subject: Re: [PATCH 1/1] ACPI: fix acpi table use after free
Message-ID: <YEe2SAESEaEak+HB@linux.ibm.com>
References: <1614802160-29362-1-git-send-email-george.kennedy@oracle.com>
 <CAJZ5v0j3=82x1hV9SCdinJQPkDXmJd9BFoqvNxNHSb6iS8PHVQ@mail.gmail.com>
 <9c3bc1b2-bb8d-194d-6faf-e4d7d346dc9b@oracle.com>
 <CAJZ5v0j8udd0R6A1wwpNvZL5Dr1pRcdiZr2if5y50o7OkHOMqg@mail.gmail.com>
 <YESEymRQ2/F7xJGt@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YESEymRQ2/F7xJGt@linux.ibm.com>
Precedence: bulk

On Sun, Mar 07, 2021 at 09:46:22AM +0200, Mike Rapoport wrote:
> Hello Rafael,
> 
> On Fri, Mar 05, 2021 at 02:30:07PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Mar 5, 2021 at 12:14 AM George Kennedy <george.kennedy@oracle.com> wrote:
> >
> > > The ibft table, for example, is mapped in via acpi_map() and kmap(). The
> > > page for the ibft table is not reserved, so it can end up on the freelist.
> > 
> > You appear to be saying that it is not sufficient to kmap() a page in
> > order to use it safely.  It is also necessary to reserve it upfront,
> > for example with the help of memblock_reserve().  Is that correct?  If
> > so, is there an alternative way to reserve a page frame?
> 
> Like David said in the other reply, if a BIOS does not mark the memory that
> contains an ACPI table as used (e.g. reserved or ACPI data), we need to
> make sure the kernel knows that such memory is in use and an early call to
> memblock_reserve() is exactly what we need here.
> George had this issue with iBFT, but in general this could be any table
> that a buggy BIOS forgot to mark as ACPI data.
  
BTW, I wonder is there a fundamental reason to use ioremap() to access ACPI
tables at all? 
In the end, they reside in RAM and, apparently, they live at the same DIMM
as neighboring "normal memory" so why cannot we just map them normally as
read-only not executable?


> > > >> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > > >> index d883176..97deea3 100644
> > > >> --- a/arch/x86/kernel/setup.c
> > > >> +++ b/arch/x86/kernel/setup.c
> > > >> @@ -1046,6 +1046,7 @@ void __init setup_arch(char **cmdline_p)
> > > >>          cleanup_highmap();
> > > >>
> > > >>          memblock_set_current_limit(ISA_END_ADDRESS);
> > > >> +       acpi_boot_table_init();
> > > > This cannot be moved before the acpi_table_upgrade() invocation AFAICS.
> > > >
> > > > Why exactly do you want to move it?
> > >
> > > Want to make sure there are slots for memblock_reserve() to be able to
> > > reserve the page.
> > 
> > Well, that may not require reordering the initialization this way.
> 
> The memory that is used by the firmware should be reserved before memblock
> allocations are allowed so that ACPI tables won't get trampled by some
> random allocation.
> 
> On x86 this essentially means that the early reservations need to be
> complete before the call to e820__memblock_setup().
> 
> We probably need more precise refactoring of ACPI init than simply moving
> acpi_boot_table_init() earlier. 
>  
> > > >>          e820__memblock_setup();
> > > >>
> 
> -- 
> Sincerely yours,
> Mike.

-- 
Sincerely yours,
Mike.