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().
>
>> +
>> + 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 at linux.ibm.com IBM Australia Limited