Re: [PATCH] cmd: sf: prevent overwriting the reserved memory

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3033 bytes --]

On Thu, Aug 08, 2024 at 01:18:44PM +0200, Michal Simek wrote:
> 
> 
> On 8/8/24 08:22, Sughosh Ganu wrote:
> > On Thu, 8 Aug 2024 at 11:05, Michal Simek <michal.simek@amd.com> wrote:
> > > 
> > > 
> > > 
> > > On 8/7/24 23:12, Tom Rini wrote:
> > > > On Tue, Aug 06, 2024 at 05:37:00PM +0530, Prasad Kummari wrote:
> > > > 
> > > > > Added LMB API to prevent SF command from overwriting reserved
> > > > > memory areas. The current SPI code does not use LMB APIs for
> > > > > loading data into memory addresses. To resolve this, LMB APIs
> > > > > were added to check the load address of an SF command and ensure it
> > > > > does not overwrite reserved memory addresses. Similar checks are
> > > > > used in TFTP, serial load, and boot code to prevent overwriting
> > > > > reserved memory.
> > > > > 
> > > > > Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
> > > > 
> > > > This is a much more generic issue that should be looked in to with the
> > > > LMB rewrite that Sughosh is working on.
> > > 
> > > yes. And is it going to be the part of his series?
> > > I expect that if he accepts this will be done on the top of it and there is
> > > likely no reason to wait.
> > 
> > This change would be needed, but in a different form. The patch, since
> > based on the current master branch, is assuming a local lmb memory
> > map. My series is doing away with that, and so we will no longer have
> > the lmb_init_and_reserve() API, for example. I would suggest that the
> > patch be put out either on top of my patches, or ideally, once the lmb
> > patches get merged.
> 
> I care that we can't overwrite reserved memory by any of load commands.
> Better to be fixed earlier rather than later but up to Tom to decide.
> From my perspective this is incorrect behavior which is fixing issue and
> likely this can go to 2024.10 version.
> Your LMB series is likely going to target 2025.01.

So, from my point of view, this is a longstanding issue that I get why
people are concerned, but I think it's missing a bigger point. For
network loads, OK, no one needs physical access to do something
malicious, so yes, it's important we check there every time. For
filesystem loads? There's far far too many production devices using SD
cards, so yes, a malicious actor needs physical access, but not much.
For flash (SPI or NAND), at that point why doesn't the malicious actor
just use "mw" instead? The device is in their position if they're able
to hook up probes/etc.

So my current thought process is that yes, fixing SPI and NAND and all
of the other forms of reading (outside of "cp") need to be fixed, as a
follow-up series to what Sughosh is doing. And then reminding people
that CMD_MEMORY is dangerous and perhaps think about splitting mw/etc
out from "cmp/base/loop" so that CMD_MEMORY can be disabled for boards
that want a more secure feel.

I'm open to being convinced I'm wrong and this is a serious problem to
address now, not later, however.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]