Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Nathan Lynch]

Michael Ellerman <mpe@xxxxxxxxxxxxxx> writes:
> Paul Moore <paul@xxxxxxxxxxxxxx> writes:
>> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@xxxxxxxxxxxxx> wrote:
>>>
>>> The error injection facility on pseries VMs allows corruption of
>>> arbitrary guest memory, potentially enabling a sufficiently privileged
>>> user to disable lockdown or perform other modifications of the running
>>> kernel via the rtas syscall.
>>>
>>> Block the PAPR error injection facility from being opened or called
>>> when locked down.
>>>
>>> Signed-off-by: Nathan Lynch <nathanl@xxxxxxxxxxxxx>
>>> ---
>>>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
>>>  include/linux/security.h   |  1 +
>>>  security/security.c        |  1 +
>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> ...
>>
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 1ca8dbacd3cc..b5d5138ae66a 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -123,6 +123,7 @@ enum lockdown_reason {
>>>         LOCKDOWN_BPF_WRITE_USER,
>>>         LOCKDOWN_DBG_WRITE_KERNEL,
>>>         LOCKDOWN_DEVICE_TREE,
>>> +       LOCKDOWN_RTAS_ERROR_INJECTION,
>>
>> With the understanding that I've never heard of RTAS until now, are
>> there any other RTAS events that would require a lockdown reason?  As
>> a follow up, is it important to distinguish between different RTAS
>> lockdown reasons?

1. Not to my current knowledge.
2. Yes, I think so, see below.

>>
>> I'm trying to determine if we can just call it LOCKDOWN_RTAS.
>
> Yes I think we should.
>
> Currently it only locks down the error injection calls, not all of RTAS.
>
> But firmware can/will add new RTAS calls in future, so it's always
> possible something will need to be added to the list of things that need
> to be blocked during lockdown.
>
> So I think calling it LOCKDOWN_RTAS would be more general and future
> proof, and can be read to mean "lockdown the parts of RTAS that need
> to be locked down".

RTAS provides callable interfaces for a broad variety of functions,
including device configuration, halt/reboot/suspend, CPU online/offline,
NVRAM access, firmware upgrade, platform diagnostic data retrieval, and
others.

Currently I don't know of other RTAS-provided functions that should be
restricted. But if we were to add more, then -- in answer to Paul -- yes
I think it would be important to have distinct reasons and
messages. Taking the point of view of someone diagnosing lockdown denial
messages and relating them to kernel code and user space activity, I
would rather we err toward specificity.

Also: LOCKDOWN_RTAS_ERROR_INJECTION is proposed for lockdown's integrity
mode. But consider that future RTAS-related additions could be proposed
for confidentiality mode, which is more restrictive. (A plausible
example could be platform dump retrieval.) We would need at least two
RTAS reasons and one wouldn't suffice.

So a single RTAS catch-all lockdown reason doesn't appeal to me, but
lockdown reasons and messages aren't ABI (right?) and we could
eventually change whatever decision we reach here. So if you both still
prefer a single LOCKDOWN_RTAS reason, I can do that for v2.

That said, I could see a case for instead adding
LOCKDOWN_HW_ERROR_INJECTION (without the RTAS), to indicate restriction
of hardware- or platform-level error injection. I was a little surprised
to find that an error injection reason doesn't already exist for the
ACPI-based facility (drivers/acpi/apei/einj.c), but since the user
interface is debugfs-based I suppose it's already restricted in practice
by LOCKDOWN_DEBUGFS.