From: Daniel Axtens <dja@axtens•net>
To: Andrew Donnellan <ajd@linux•ibm.com>,
Michael Ellerman <mpe@ellerman•id.au>,
linuxppc-dev@lists•ozlabs.org
Cc: nathanl@linux•ibm.com, leobras.c@gmail•com
Subject: Re: [PATCH] powerpc/rtas: Restrict RTAS requests from userspace
Date: Tue, 11 Aug 2020 23:41:12 +1000 [thread overview]
Message-ID: <875z9pnvuv.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <1ff85ddd-1b75-f49d-0ae2-edf9e5a199e2@linux.ibm.com>
Andrew Donnellan <ajd@linux•ibm.com> writes:
> On 10/8/20 4:40 pm, Michael Ellerman wrote:
>> Hi ajd,
>>
>> Thanks for taking care of this.
>>
>> I was going to merge this as-is, but given it's fixing a long standing
>> issue there's not really a big rush. So a few comments below.
>
> Thanks for the review.
>
>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>> index a09eba03f180..ec1cae52d8bd 100644
>>> --- a/arch/powerpc/kernel/rtas.c
>>> +++ b/arch/powerpc/kernel/rtas.c
>>> @@ -324,6 +324,23 @@ int rtas_token(const char *service)
>>> }
>>> EXPORT_SYMBOL(rtas_token);
>>>
>>> +#ifdef CONFIG_PPC_RTAS_FILTER
>>> +
>>
>> I think this could be combined with the #ifdef block below?
>
> I put it here to keep it next to rtas_token() as it seemed thematically
> appropriate. Anyway per below I'll probably get rid of this function
> altogether.
>
>>
>>> +static char *rtas_token_name(int token)
>>> +{
>>> + struct property *prop;
>>> +
>>> + for_each_property_of_node(rtas.dev, prop) {
>>> + const __be32 *tokp = prop->value;
>>> +
>>> + if (tokp && be32_to_cpu(*tokp) == token)
>>> + return prop->name;
>>> + }
>>> + return NULL;
>>> +}
>>> +
>>> +#endif /* CONFIG_PPC_RTAS_FILTER */
>>> +
>>> int rtas_service_present(const char *service)
>>> {
>>> return rtas_token(service) != RTAS_UNKNOWN_SERVICE;
>>> @@ -1110,6 +1127,184 @@ struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log,
>>> return NULL;
>>> }
>>>
>>> +#ifdef CONFIG_PPC_RTAS_FILTER
>>> +
>>> +/*
>>> + * The sys_rtas syscall, as originally designed, allows root to pass
>>> + * arbitrary physical addresses to RTAS calls. A number of RTAS calls
>>> + * can be abused to write to arbitrary memory and do other things that
>>> + * are potentially harmful to system integrity, and thus should only
>>> + * be used inside the kernel and not exposed to userspace.
>>> + *
>>> + * All known legitimate users of the sys_rtas syscall will only ever
>>> + * pass addresses that fall within the RMO buffer, and use a known
>>> + * subset of RTAS calls.
>>> + *
>>> + * Accordingly, we filter RTAS requests to check that the call is
>>> + * permitted, and that provided pointers fall within the RMO buffer.
>>> + * The rtas_filters list contains an entry for each permitted call,
>>> + * with the indexes of the parameters which are expected to contain
>>> + * addresses and sizes of buffers allocated inside the RMO buffer.
>>> + */
>>> +struct rtas_filter {
>>> + const char name[32];
>>
>> Using a const char * for the name would be more typical, meaning the
>> strings would end up in .rodata, and could be merged with other uses of
>> the same strings.
>
> Will fix
>
>>
>>> +
>>> + /* Indexes into the args buffer, -1 if not used */
>>> + int rmo_buf_idx1;
>>> + int rmo_size_idx1;
>>> + int rmo_buf_idx2;
>>> + int rmo_size_idx2;
>>
>> The "rmo" prefix is probably unnecessary?
>>
>
> Ack
>
>>> +};
>>> +
>>> +struct rtas_filter rtas_filters[] = {
>>
>> Should be static, and __ro_after_init ?
>>
>>> + { "ibm,activate-firmware", -1, -1, -1, -1 },
>>
>> Would it be worth making the indices 1-based, allowing 0 to be the
>> unused value, meaning you only have to initialise the used fields?
>
> 1-based array indices are morally reprehensible. It would make it
> cleaner but I kind of prefer an obvious and clear constant for unused, idk
>
>> It would require adjusting them before use, but there's only 4 places
>> they're used, and you could probably use a macro to do the - 1.
>>
>>> + { "ibm,configure-connector", 0, -1, 1, -1 }, /* Special cased, size 4096 */
>>
>> Does it make sense to put the hard coded sizes in the structure as well?
>>
>> eg. fixed_size1 = 4096,
>>
>> I think that would avoid the need for any strcmps in the code.
>
> Not quite - we still have a special case for ibm,configure-connector
> passing a base address of 0.
>
> But yes that's a good idea.
>
>>
>>> + { "display-character", -1, -1, -1, -1 },
>>> + { "ibm,display-message", 0, -1, -1, -1 },
>>> + { "ibm,errinjct", 2, -1, -1, -1 }, /* Fixed size of 1024 */
>>> + { "ibm,close-errinjct", -1, -1, -1, -1 },
>>> + { "ibm,open-errinct", -1, -1, -1, -1 },
>>> + { "ibm,get-config-addr-info2", -1, -1, -1, -1 },
>>> + { "ibm,get-dynamic-sensor-state", 1, -1, -1, -1 },
>>> + { "ibm,get-indices", 2, 3, -1, -1 },
>>> + { "get-power-level", -1, -1, -1, -1 },
>>> + { "get-sensor-state", -1, -1, -1, -1 },
>>> + { "ibm,get-system-parameter", 1, 2, -1, -1 },
>>> + { "get-time-of-day", -1, -1, -1, -1 },
>>> + { "ibm,get-vpd", 0, -1, 1, 2 },
>>> + { "ibm,lpar-perftools", 2, 3, -1, -1 },
>>> + { "ibm,platform-dump", 4, 5, -1, -1 },
>>> + { "ibm,read-slot-reset-state", -1, -1, -1, -1 },
>>> + { "ibm,scan-log-dump", 0, 1, -1, -1 },
>>> + { "ibm,set-dynamic-indicator", 2, -1, -1, -1 },
>>> + { "ibm,set-eeh-option", -1, -1, -1, -1 },
>>> + { "set-indicator", -1, -1, -1, -1 },
>>> + { "set-power-level", -1, -1, -1, -1 },
>>> + { "set-time-for-power-on", -1, -1, -1, -1 },
>>> + { "ibm,set-system-parameter", 1, -1, -1, -1 },
>>> + { "set-time-of-day", -1, -1, -1, -1 },
>>> + { "ibm,suspend-me", -1, -1, -1, -1 },
>>> + { "ibm,update-nodes", 0, -1, -1, -1 }, /* Fixed size of 4096 */
>>> + { "ibm,update-properties", 0, -1, -1, -1 }, /* Fixed size of 4096 */
>>> + { "ibm,physical-attestation", 0, 1, -1, -1 },
>>> +};
>>> +
>>> +static void dump_rtas_params(int token, int nargs, int nret,
>>> + struct rtas_args *args)
>>> +{
>>> + int i;
>>> + char *token_name = rtas_token_name(token);
>>> +
>>> + pr_err_ratelimited("sys_rtas: token=0x%x (%s), nargs=%d, nret=%d (called by %s)\n",
>>> + token, token_name ? token_name : "unknown", nargs,
>>> + nret, current->comm);
>>> + pr_err_ratelimited("sys_rtas: args: ");
>>> +
>>> + for (i = 0; i < nargs; i++) {
>>> + u32 arg = be32_to_cpu(args->args[i]);
>>> +
>>> + pr_cont("%08x ", arg);
>>> + if (arg >= rtas_rmo_buf &&
>>> + arg < (rtas_rmo_buf + RTAS_RMOBUF_MAX))
>>> + pr_cont("(buf+0x%lx) ", arg - rtas_rmo_buf);
>>> + }
>>
>> This can leak the location of the RMO buf into dmesg. I know it's
>> visible via /proc, but the /proc file is 0400.
>>
>> So I think it's probably safer if we just don't dump the args, or their
>> relation to the RMO buf.
>
> Good point, removed.
>
>>
>>> +
>>> + pr_cont("\n");
>>> +}
>>> +
>>> +static bool in_rmo_buf(u32 base, u32 end)
>>> +{
>>> + return base >= rtas_rmo_buf &&
>>> + base < (rtas_rmo_buf + RTAS_RMOBUF_MAX) &&
>>> + base <= end &&
>>> + end >= rtas_rmo_buf &&
>>> + end < (rtas_rmo_buf + RTAS_RMOBUF_MAX);
>>> +}
>>> +
>>> +static bool block_rtas_call(int token, int nargs,
>>> + struct rtas_args *args)
>>> +{
>>> + int i;
>>> + const char *reason;
>>> + char *token_name = rtas_token_name(token);
>>
>> This code isn't particularly performance critical, but I think it would
>> be cleaner to do the token lookup once at init time, and store the token
>> in the filter array?
>>
>> Then this code would only be doing token comparisons.
>
> Yeah that would be cleaner, can get rid of rtas_token_name().
I'm not sure I quite understand what you're suggesting.
You still need to do a string->token lookup at least once as the tokens
differ between PowerVM and qemu. Are you saying that you can fold the
token name lookup into the init function?
Kind regards,
Daniel
>>
>>> +
>>> + if (!token_name)
>>> + goto err_notpermitted;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
>>> + struct rtas_filter *f = &rtas_filters[i];
>>> + u32 base, size, end;
>>> +
>>> + if (strcmp(token_name, f->name))
>>> + continue;
>>> +
>>> + if (f->rmo_buf_idx1 != -1) {
>>> + base = be32_to_cpu(args->args[f->rmo_buf_idx1]);
>>> + if (f->rmo_size_idx1 != -1)
>>> + size = be32_to_cpu(args->args[f->rmo_size_idx1]);
>>> + else if (!strcmp(token_name, "ibm,errinjct"))
>>> + size = 1024;
>>> + else if (!strcmp(token_name, "ibm,update-nodes") ||
>>> + !strcmp(token_name, "ibm,update-properties") ||
>>> + !strcmp(token_name, "ibm,configure-connector"))
>>> + size = 4096;
>>> + else
>>> + size = 1;
>>> +
>>> + end = base + size - 1;
>>> + if (!in_rmo_buf(base, end)) {
>>> + reason = "address pair 1 out of range";
>>
>> I don't think we need to give the user this much detail about what they
>> did wrong, all cases can just print "call not permitted" IMO.
>
> Ack
>
> --
> Andrew Donnellan OzLabs, ADL Canberra
> ajd@linux•ibm.com IBM Australia Limited
next prev parent reply other threads:[~2020-08-11 13:43 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 16:19 [PATCH] powerpc/rtas: Restrict RTAS requests from userspace Andrew Donnellan
2020-07-10 14:02 ` Sasha Levin
2020-07-27 1:23 ` Daniel Axtens
2020-08-10 6:40 ` Michael Ellerman
2020-08-11 8:04 ` Andrew Donnellan
2020-08-11 11:48 ` Michael Ellerman
2020-08-11 13:41 ` Daniel Axtens [this message]
2020-08-12 3:13 ` Andrew Donnellan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875z9pnvuv.fsf@dja-thinkpad.axtens.net \
--to=dja@axtens$(echo .)net \
--cc=ajd@linux$(echo .)ibm.com \
--cc=leobras.c@gmail$(echo .)com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=mpe@ellerman$(echo .)id.au \
--cc=nathanl@linux$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox