Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 16:24:42 -0700
From:   Kees Cook <keescook@chromium.org>
To:     "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc:     Yuxiao Zhang <yuxiaozhang@google.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Tony Luck <tony.luck@intel.com>,
        linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
        wak@google.com
Subject: Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc
 limitation
Message-ID: <202306281615.360B12EFC@keescook>
References: <2023062832-barrier-strenuous-9499@gregkh>
 <20230628171055.1312737-1-yuxiaozhang@google.com>
 <28718037-ccf1-c36b-4eb6-716c518ca98f@igalia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <28718037-ccf1-c36b-4eb6-716c518ca98f@igalia.com>
Precedence: bulk

On Wed, Jun 28, 2023 at 03:12:10PM -0300, Guilherme G. Piccoli wrote:
> Also - Kees certainly knows that way better, but - are we 100% sure that
> the region for pstore can be non-contiguous? For some reason, I always
> thought this was a requirement - grepping the code, I found this
> (wrong?) comment:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/pstore/zone.c#n3

The contiguous requirements are entirely for the RAM backend's storage,
so these intermediate memory regions can use non-contiguous physical
backing memory (i.e. vmalloc).

Even the special case of crash-dumping should be fine for the large
buffer used to hold the crash before doing compression.

There are effectively 4 types of allocations in pstore:

1- a physical -> virtual mapping for the RAM backend
2- the allocations (if any) for non-RAM backends to hold a copy of pstore
   records when extracted from the backend storage (e.g NVRAM, EFI vars,
   etc).
3- incoming allocations that are used temporarily to hand data to the
   backend (e.g. the write_compat used in this patch)
4- the allocation used for holding the pstorefs data contents (which I
   need to double-check, but is entirely defined by the backends)

Changes aren't needed for (1), it's fine on its own. This patch changes
allocations for (2) and (3) for pmsg and the RAM backend, if I'm reading
it correctly. I think (4) is included as part of (2), but I need to
check. As long as all paths use kvfree() for the record buffers,
everything should Just Work for RAM. Switching the other backends to
also use kvalloc() would allow for greater flexibility, though.

Anyway, it's on my list to review and test. I'm still working through
other things related to the merge window opening, so I may be a bit slow
for the next week. :)

-Kees

-- 
Kees Cook