linux-kernel - Re: [PATCH] ARM: tegra: add basic SecureOS support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAVeFu+by44HnOzv_85kwgeCx5b9TxiMhr27x69QcUj9GRbk8A@mail.gmail.com>
Date:	Fri, 7 Jun 2013 17:11:40 +0900
From:	Alexandre Courbot <gnurou@...il.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Alexandre Courbot <acourbot@...dia.com>,
	Joseph Lo <josephl@...dia.com>,
	Karan Jhavar <kjhavar@...dia.com>,
	Varun Wadekar <vwadekar@...dia.com>,
	Chris Johnson <CJohnson@...dia.com>,
	Matthew Longnecker <MLongnecker@...dia.com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] ARM: tegra: add basic SecureOS support

On Fri, Jun 7, 2013 at 1:44 AM, Stephen Warren <swarren@...dotorg.org> wrote:
> On 06/06/2013 01:28 AM, Alexandre Courbot wrote:
>> Boot loaders on some Tegra devices can be unlocked but do not let the
>> system operate without SecureOS. SecureOS prevents access to some
>> registers and requires the operating system to perform certain
>> operations through Secure Monitor Calls instead of directly accessing
>> the hardware.
>>
>> This patch introduces basic SecureOS support for Tegra. SecureOS support
>> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
>> node of the device tree.
>
> I suspect "SecureOS" here is the name of a specific implementation of a
> secure monitor, right? It's certainly a very unfortunate name, since it
> sounds like a generic concept rather than a specific implementation:-(

Right. Using the actual name (Trusted Foundations) is probably better.
I don't think the SecureOS denomination is used by anyone else but
NVIDIA.

> There certainly could be (and I believe are in practice?) multiple
> implementation of a secure monitor for Tegra. Presumably, each of those
> implementations has (or could have) a different definition for what SVC
> calls it supports, their parameters, etc.
>
> I think we need to separate the concept of support for *a* secure
> monitor, from support for a *particular* secure monitor.

Agreed. In this case, can we assume that support for a specific secure
monitor is not arch-specific, and that this patch should be moved
outside of arch-tegra and down to arch/arm? In other words, the ABI of
a particular secure monitor should be the same no matter the chip,
shouldn't it?

>> +node:
>> +
>> +  nvidia,secure-os: enable SecureOS.
>
> ... as such, I think some work is needed here to allow specification of
> which secure monitor is present and/or which secure monitor ABI it
> implements. The suggestion to use a specific DT node, and hence use
> compatible values for this, seems reasonable. Alternatively, perhaps:
>
> nvidia,secure-monitor = "name_of_vendor_or_SMC_ABI";
>
> might be reasonable, although using a node would allow ABI-specific
> additional properties to be defined should they be needed, so I guess I
> would lean towards that.

Considering your previous comment, I agree this seems to make the most sense.

> Similar comments may apply to the CONFIG_ option and description,
> filenames, etc.
>
>> diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c
>
>> +void __init tegra_init_secureos(void)
>> +{
>> +     struct device_node *node = of_find_node_by_path("/chosen");
>> +
>> +     if (node && of_property_read_bool(node, "nvidia,secure-os"))
>> +             register_firmware_ops(&tegra_firmware_ops);
>> +}
>
> I'm tempted to think that function should /always/ exist an be called
> (so no dummy inline in secureos.h).
>
> In particular, what happens when a kernel without CONFIG_SECUREOS
> enabled is booted under a secure monitor. Presumably we want the init
> code to explicitly check for this condition, and either BUG(), or do
> something to disable any features (like SMP) that require SVCs?
>
> So, the algorithm here might be more like:
>
> find node
> if node exists
>     if (!IS_ENABLED(SECURE_OS))
>         BUG/WARN/...
>     else
>         register_firmware_ops()
>
> ?

Yep, let's do it this way.

Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/