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:   Sat, 3 Dec 2022 20:37:51 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     Dionna Amalie Glaze <dionnaglaze@google.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        Peter Gonda <pgonda@google.com>,
        Thomas Lendacky <Thomas.Lendacky@amd.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Joerg Roedel <jroedel@suse.de>, Ingo Molnar <mingo@redhat.com>,
        Andy Lutomirsky <luto@kernel.org>,
        John Allen <john.allen@amd.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v8 1/4] crypto: ccp - Name -1 return value as
 SEV_RET_NO_FW_CALL
Message-ID: <Y4ulj38eMr1NiRdX@zn.tnic>
References: <20221104230040.2346862-1-dionnaglaze@google.com>
 <20221104230040.2346862-2-dionnaglaze@google.com>
 <Y4tAX580jEGHOU9d@zn.tnic>
 <CAAH4kHYz-46syE4wKPzo1N9P34wLHcs85obOCjqb6eQ=iv=n3w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAAH4kHYz-46syE4wKPzo1N9P34wLHcs85obOCjqb6eQ=iv=n3w@mail.gmail.com>
Precedence: bulk

On Sat, Dec 03, 2022 at 10:58:39AM -0800, Dionna Amalie Glaze wrote:
> It doesn't always overwrite psp_ret, such as the initial error checking.
> The value remains uninitialized for -ENODEV, -EBUSY, -EINVAL.
> Thus *error in __sev_platform_init_locked can be set to uninitialized
> memory if psp_ret is not first initialized.

Lemme see if I understand it correctly: you wanna signal that all early
return cases in __sev_do_cmd_locked() are such that no firmware was
called?

I.e., everything before the first iowrite into the command buffer?

But then the commit message says:

"The PSP can return a "firmware error" code of -1 in circumstances where
the PSP is not actually called."

which is confusing. How can the PSP return something if it wasn't called?

Or you mean those cases above where it would fail on some of the checks
before issuing a SEV command? I think you do...

So I see Tom has ACKed this but I have to ask: is the SEV spec not going
to use -1 ever?

Also, if this behavior is going to be user-visible, where are we
documenting it? Especially if nothing in the kernel is looking at
that value but only assigning it to a retval which gets looked at by
userspace. Especially then this should be documented.

Dunno, maybe somewhere in Documentation/x86/amd-memory-encryption.rst or
maybe Tom would have a better idea.

> That error points to the kernel copy of the user's argument struct,
> which the ioctl always copies back. In the case of those error codes
> then, without SEV_RET_NO_FW_CALL, user space will get uninitialized
> kernel memory.

Right, but having a return value which means "firmware wasn't called"
sounds weird. Why does userspace care?

I mean, you can just as well return any of the negative values -ENODEV,
-EBUSY, -EINVAL too, depending on where you exit. Having three different
retvals could tell you where exactly it failed, even.

But the question remains: why does userspace needs to know that the
failure happened and firmware wasn't called, as long as it is getting
something negative to signal an error?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette