* Re: [PATCH v4 18/28] docs: convert docs to ReST and rename to *.rst
From: Mauro Carvalho Chehab @ 2019-06-13 9:58 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Sebastian Reichel, Rafael J. Wysocki,
Viresh Kumar, Len Brown, Pavel Machek, Nishanth Menon,
Stephen Boyd, Liam Girdwood, Mark Brown, Mathieu Poirier,
Suzuki K Poulose, Harry Wei, Alex Shi, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, David Airlie, Daniel Vetter,
Bjorn Helgaas, Johannes Berg, David S. Miller, linux-pm,
linux-arm-kernel, intel-gfx, dri-devel, linux-pci, linux-wireless,
netdev
In-Reply-To: <7dc94cb4-ebf1-22ab-29c9-fcb2b875a9ac@csail.mit.edu>
Em Wed, 12 Jun 2019 17:25:39 -0700
"Srivatsa S. Bhat" <srivatsa@csail.mit.edu> escreveu:
> On 6/12/19 10:52 AM, Mauro Carvalho Chehab wrote:
> > Convert the PM documents to ReST, in order to allow them to
> > build with Sphinx.
> >
> > The conversion is actually:
> > - add blank lines and identation in order to identify paragraphs;
> > - fix tables markups;
> > - add some lists markups;
> > - mark literal blocks;
> > - adjust title markups.
> >
> > At its new index.rst, let's add a :orphan: while this is not linked to
> > the main index.rst file, in order to avoid build warnings.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Acked-by: Mark Brown <broonie@kernel.org>
> > ---
>
> [...]
>
> > diff --git a/Documentation/power/suspend-and-cpuhotplug.txt b/Documentation/power/suspend-and-cpuhotplug.rst
> > similarity index 90%
> > rename from Documentation/power/suspend-and-cpuhotplug.txt
> > rename to Documentation/power/suspend-and-cpuhotplug.rst
> > index a8751b8df10e..9df664f5423a 100644
> > --- a/Documentation/power/suspend-and-cpuhotplug.txt
> > +++ b/Documentation/power/suspend-and-cpuhotplug.rst
> > @@ -1,10 +1,15 @@
> > +====================================================================
> > Interaction of Suspend code (S3) with the CPU hotplug infrastructure
> > +====================================================================
> >
> > - (C) 2011 - 2014 Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > +(C) 2011 - 2014 Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> >
> >
> > -I. How does the regular CPU hotplug code differ from how the Suspend-to-RAM
> > - infrastructure uses it internally? And where do they share common code?
> > +I. Differences between CPU hotplug and Suspend-to-RAM
> > +======================================================
> > +
> > +How does the regular CPU hotplug code differ from how the Suspend-to-RAM
> > +infrastructure uses it internally? And where do they share common code?
> >
> > Well, a picture is worth a thousand words... So ASCII art follows :-)
> >
>
> [...]
>
> > @@ -101,7 +108,7 @@ execution during resume):
> >
> > It is to be noted here that the system_transition_mutex lock is acquired at the very
> > beginning, when we are just starting out to suspend, and then released only
> > -after the entire cycle is complete (i.e., suspend + resume).
> > +after the entire cycle is complete (i.e., suspend + resume)::
> >
>
> I think that should be a period, not a colon, because it is clarifying
> the text above it (as opposed to referring to the example below it).
>
> Other than that, for suspend-and-cpuhotplug.txt:
>
> Acked-by: Srivatsa S. Bhat (VMware) <srivatsa@csail.mit.edu>
Ah, ok. I'll change it to:
after the entire cycle is complete (i.e., suspend + resume).
::
and add your acked-by.
>
> Regards,
> Srivatsa
> VMware Photon OS
Thanks,
Mauro
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Vincenzo Frascino @ 2019-06-13 10:15 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arm-kernel, linux-doc, linux-mm, linux-arch,
linux-kselftest, linux-kernel, Will Deacon, Andrey Konovalov,
Alexander Viro, Szabolcs Nagy
In-Reply-To: <20190612153538.GL28951@C02TF0J2HF1T.local>
Hi Catalin,
On 12/06/2019 16:35, Catalin Marinas wrote:
> Hi Vincenzo,
>
> Some minor comments below but it looks fine to me overall. Cc'ing
> Szabolcs as well since I'd like a view from the libc people.
>
Thanks for this, I saw Szabolcs comments.
> On Wed, Jun 12, 2019 at 03:21:10PM +0100, Vincenzo Frascino wrote:
>> diff --git a/Documentation/arm64/tagged-address-abi.txt b/Documentation/arm64/tagged-address-abi.txt
>> new file mode 100644
>> index 000000000000..96e149e2c55c
>> --- /dev/null
>> +++ b/Documentation/arm64/tagged-address-abi.txt
>> @@ -0,0 +1,111 @@
>> +ARM64 TAGGED ADDRESS ABI
>> +========================
>> +
>> +This document describes the usage and semantics of the Tagged Address
>> +ABI on arm64.
>> +
>> +1. Introduction
>> +---------------
>> +
>> +On arm64 the TCR_EL1.TBI0 bit has been always enabled on the arm64 kernel,
>> +hence the userspace (EL0) is allowed to set a non-zero value in the top
>
> I'd be clearer here: "userspace (EL0) is allowed to perform a user
> memory access through a 64-bit pointer with a non-zero top byte" (or
> something along the lines). Otherwise setting a non-zero top byte is
> allowed on any architecture, dereferencing it is a problem.
>
Ok.
>> +byte but the resulting pointers are not allowed at the user-kernel syscall
>> +ABI boundary.
>> +
>> +This document describes a relaxation of the ABI with which it is possible
>
> "relaxation of the ABI that makes it possible to..."
>
>> +to pass tagged tagged pointers to the syscalls, when these pointers are in
>> +memory ranges obtained as described in paragraph 2.
>
> "section 2" is better. There are a lot more paragraphs.
>
Agree.
>> +
>> +Since it is not desirable to relax the ABI to allow tagged user addresses
>> +into the kernel indiscriminately, arm64 provides a new sysctl interface
>> +(/proc/sys/abi/tagged_addr) that is used to prevent the applications from
>> +enabling the relaxed ABI and a new prctl() interface that can be used to
>> +enable or disable the relaxed ABI.
>> +
>> +The sysctl is meant also for testing purposes in order to provide a simple
>> +way for the userspace to verify the return error checking of the prctl()
>> +command without having to reconfigure the kernel.
>> +
>> +The ABI properties are inherited by threads of the same application and
>> +fork()'ed children but cleared when a new process is spawn (execve()).
>
> "spawned".
>
> I guess you could drop these three paragraphs here and mention the
> inheritance properties when introducing the prctl() below. You can also
> mention the global sysctl switch after the prctl() was introduced.
>
I will move the last two (rewording them) to the _section_ 2, but I would still
prefer the Introduction to give an overview of the solution as well.
>> +
>> +2. ARM64 Tagged Address ABI
>> +---------------------------
>> +
>> +From the kernel syscall interface prospective, we define, for the purposes
>> +of this document, a "valid tagged pointer" as a pointer that either it has
>
> "either has" (no 'it') sounds slightly better but I'm not a native
> English speaker either.
>
>> +a zero value set in the top byte or it has a non-zero value, it is in memory
>> +ranges privately owned by a userspace process and it is obtained in one of
>> +the following ways:
>> + - mmap() done by the process itself, where either:
>> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
>> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
>> + file or "/dev/zero"
>> + - a mapping below sbrk(0) done by the process itself
>> + - any memory mapped by the kernel in the process's address space during
>> + creation and following the restrictions presented above (i.e. data, bss,
>> + stack).
>> +
>> +The ARM64 Tagged Address ABI is an opt-in feature, and an application can
>> +control it using the following prctl()s:
>> + - PR_SET_TAGGED_ADDR_CTRL: can be used to enable the Tagged Address ABI.
>
> enable or disable (not sure we need the latter but it doesn't heart).
>
> I'd add the arg2 description here as well.
>
Good point I missed this.
>> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
>> + Address ABI.
>> +
>> +As a consequence of invoking PR_SET_TAGGED_ADDR_CTRL prctl() by an applications,
>> +the ABI guarantees the following behaviours:
>> +
>> + - Every current or newly introduced syscall can accept any valid tagged
>> + pointers.
>> +
>> + - If a non valid tagged pointer is passed to a syscall then the behaviour
>> + is undefined.
>> +
>> + - Every valid tagged pointer is expected to work as an untagged one.
>> +
>> + - The kernel preserves any valid tagged pointers and returns them to the
>> + userspace unchanged in all the cases except the ones documented in the
>> + "Preserving tags" paragraph of tagged-pointers.txt.
>
> I'd think we need to qualify the context here in which the kernel
> preserves the tagged pointers. Did you mean on the syscall return?
>
What this means is that on syscall return the tags are preserved, but if for
example you have tagged pointers inside siginfo_t, they will not because
according to tagged-pointers.txt non-zero tags are not preserved when delivering
signals.
>> +
>> +A definition of the meaning of tagged pointers on arm64 can be found in:
>> +Documentation/arm64/tagged-pointers.txt.
>> +
>> +3. ARM64 Tagged Address ABI Exceptions
>> +--------------------------------------
>> +
>> +The behaviours described in paragraph 2, with particular reference to the
>
> "section 2"
>
>> +acceptance by the syscalls of any valid tagged pointer are not applicable
>> +to the following cases:
>> + - mmap() addr parameter.
>> + - mremap() new_address parameter.
>> + - prctl_set_mm() struct prctl_map fields.
>> + - prctl_set_mm_map() struct prctl_map fields.
>> +
>> +4. Example of correct usage
>> +---------------------------
>> +
>> +void main(void)
>> +{
>> + static int tbi_enabled = 0;
>> + unsigned long tag = 0;
>> +
>> + char *ptr = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE,
>> + MAP_ANONYMOUS, -1, 0);
>> +
>> + if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE,
>> + 0, 0, 0) == 0)
>> + tbi_enabled = 1;
>> +
>> + if (!ptr)
>> + return -1;
>> +
>> + if (tbi_enabled)
>> + tag = rand() & 0xff;
>> +
>> + ptr = (char *)((unsigned long)ptr | (tag << TAG_SHIFT));
>> +
>> + *ptr = 'a';
>> +
>> + ...
>> +}
>> +
>> --
>> 2.21.0
>
--
Regards,
Vincenzo
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Szabolcs Nagy @ 2019-06-13 10:14 UTC (permalink / raw)
To: Catalin Marinas
Cc: nd, Vincenzo Frascino, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, linux-mm@kvack.org,
linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, Will Deacon, Andrey Konovalov,
Alexander Viro
In-Reply-To: <20190613092054.GO28951@C02TF0J2HF1T.local>
On 13/06/2019 10:20, Catalin Marinas wrote:
> Hi Szabolcs,
>
> On Wed, Jun 12, 2019 at 05:30:34PM +0100, Szabolcs Nagy wrote:
>> On 12/06/2019 15:21, Vincenzo Frascino wrote:
>>> +2. ARM64 Tagged Address ABI
>>> +---------------------------
>>> +
>>> +From the kernel syscall interface prospective, we define, for the purposes
>> ^^^^^^^^^^^
>> perspective
>>
>>> +of this document, a "valid tagged pointer" as a pointer that either it has
>>> +a zero value set in the top byte or it has a non-zero value, it is in memory
>>> +ranges privately owned by a userspace process and it is obtained in one of
>>> +the following ways:
>>> + - mmap() done by the process itself, where either:
>>> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
>>> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
>>> + file or "/dev/zero"
>>
>> this does not make it clear if MAP_FIXED or other flags are valid
>> (there are many map flags i don't know, but at least fixed should work
>> and stack/growsdown. i'd expect anything that's not incompatible with
>> private|anon to work).
>
> Just to clarify, this document tries to define the memory ranges from
> where tagged addresses can be passed into the kernel in the context
> of TBI only (not MTE); that is for hwasan support. FIXED or GROWSDOWN
> should not affect this.
yes, so either the text should list MAP_* flags that don't affect
the pointer tagging semantics or specify private|anon mapping
with different wording.
>>> + - a mapping below sbrk(0) done by the process itself
>>
>> doesn't the mmap rule cover this?
>
> IIUC it doesn't cover it as that's memory mapped by the kernel
> automatically on access vs a pointer returned by mmap(). The statement
> above talks about how the address is obtained by the user.
ok i read 'mapping below sbrk' as an mmap (possibly MAP_FIXED)
that happens to be below the heap area.
i think "below sbrk(0)" is not the best term to use: there
may be address range below the heap area that can be mmapped
and thus below sbrk(0) and sbrk is a posix api not a linux
syscall, the libc can implement it with mmap or whatever.
i'm not sure what the right term for 'heap area' is
(the address range between syscall(__NR_brk,0) at
program startup and its current value?)
>>> + - any memory mapped by the kernel in the process's address space during
>>> + creation and following the restrictions presented above (i.e. data, bss,
>>> + stack).
>>
>> OK.
>>
>> Can a null pointer have a tag?
>> (in case NULL is valid to pass to a syscall)
>
> Good point. I don't think it can. We may change this for MTE where we
> give a hint tag but no hint address, however, this document only covers
> TBI for now.
OK.
>>> +The ARM64 Tagged Address ABI is an opt-in feature, and an application can
>>> +control it using the following prctl()s:
>>> + - PR_SET_TAGGED_ADDR_CTRL: can be used to enable the Tagged Address ABI.
>>> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
>>> + Address ABI.
>>> +
>>> +As a consequence of invoking PR_SET_TAGGED_ADDR_CTRL prctl() by an applications,
>>> +the ABI guarantees the following behaviours:
>>> +
>>> + - Every current or newly introduced syscall can accept any valid tagged
>>> + pointers.
>>> +
>>> + - If a non valid tagged pointer is passed to a syscall then the behaviour
>>> + is undefined.
>>> +
>>> + - Every valid tagged pointer is expected to work as an untagged one.
>>> +
>>> + - The kernel preserves any valid tagged pointers and returns them to the
>>> + userspace unchanged in all the cases except the ones documented in the
>>> + "Preserving tags" paragraph of tagged-pointers.txt.
>>
>> OK.
>>
>> i guess pointers of another process are not "valid tagged pointers"
>> for the current one, so e.g. in ptrace the ptracer has to clear the
>> tags before PEEK etc.
>
> Another good point. Are there any pros/cons here or use-cases? When we
> add MTE support, should we handle this differently?
i'm not sure what gdb does currently, but it has
an 'address_significant' hook used at a few places
that drops the tag on aarch64, so it probably
avoids passing tagged pointer to ptrace.
i was worried about strace which tries to print
structs passed to syscalls and follow pointers in
them which currently would work, but if we allow
tags in syscalls then it needs some update.
(i haven't checked the strace code though)
>>> +A definition of the meaning of tagged pointers on arm64 can be found in:
>>> +Documentation/arm64/tagged-pointers.txt.
>>> +
>>> +3. ARM64 Tagged Address ABI Exceptions
>>> +--------------------------------------
>>> +
>>> +The behaviours described in paragraph 2, with particular reference to the
>>> +acceptance by the syscalls of any valid tagged pointer are not applicable
>>> +to the following cases:
>>> + - mmap() addr parameter.
>>> + - mremap() new_address parameter.
>>> + - prctl_set_mm() struct prctl_map fields.
>>> + - prctl_set_mm_map() struct prctl_map fields.
>>
>> i don't understand the exception: does it mean that passing a tagged
>> address to these syscalls is undefined?
>
> I'd say it's as undefined as it is right now without these patches. We
> may be able to explain this better in the document.
>
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Vincenzo Frascino @ 2019-06-13 15:35 UTC (permalink / raw)
To: Szabolcs Nagy, Catalin Marinas
Cc: nd, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, linux-mm@kvack.org,
linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, Will Deacon, Andrey Konovalov,
Alexander Viro
In-Reply-To: <ba822b33-a822-02ef-9b85-725f4353596a@arm.com>
On 13/06/2019 16:32, Szabolcs Nagy wrote:
> On 13/06/2019 15:03, Vincenzo Frascino wrote:
>> On 13/06/2019 13:28, Szabolcs Nagy wrote:
>>> On 13/06/2019 12:16, Vincenzo Frascino wrote:
>>>> On 13/06/2019 11:14, Szabolcs Nagy wrote:
>>>>> On 13/06/2019 10:20, Catalin Marinas wrote:
>>>>>> On Wed, Jun 12, 2019 at 05:30:34PM +0100, Szabolcs Nagy wrote:
>>>>>>> On 12/06/2019 15:21, Vincenzo Frascino wrote:
>>>>>>>> + - a mapping below sbrk(0) done by the process itself
>>>>>>>
>>>>>>> doesn't the mmap rule cover this?
>>>>>>
>>>>>> IIUC it doesn't cover it as that's memory mapped by the kernel
>>>>>> automatically on access vs a pointer returned by mmap(). The statement
>>>>>> above talks about how the address is obtained by the user.
>>>>>
>>>>> ok i read 'mapping below sbrk' as an mmap (possibly MAP_FIXED)
>>>>> that happens to be below the heap area.
>>>>>
>>>>> i think "below sbrk(0)" is not the best term to use: there
>>>>> may be address range below the heap area that can be mmapped
>>>>> and thus below sbrk(0) and sbrk is a posix api not a linux
>>>>> syscall, the libc can implement it with mmap or whatever.
>>>>>
>>>>> i'm not sure what the right term for 'heap area' is
>>>>> (the address range between syscall(__NR_brk,0) at
>>>>> program startup and its current value?)
>>>>>
>>>>
>>>> I used sbrk(0) with the meaning of "end of the process's data segment" not
>>>> implying that this is a syscall, but just as a useful way to identify the mapping.
>>>> I agree that it is a posix function implemented by libc but when it is used with
>>>> 0 finds the current location of the program break, which can be changed by brk()
>>>> and depending on the new address passed to this syscall can have the effect of
>>>> allocating or deallocating memory.
>>>>
>>>> Will changing sbrk(0) with "end of the process's data segment" make it more clear?
>>>
>>> i don't understand what's the relevance of the *end*
>>> of the data segment.
>>>
>>> i'd expect the text to say something about the address
>>> range of the data segment.
>>>
>>> i can do
>>>
>>> mmap((void*)65536, 65536, PROT_READ|PROT_WRITE, MAP_FIXED|MAP_SHARED|MAP_ANON, -1, 0);
>>>
>>> and it will be below the end of the data segment.
>>>
>>
>> As far as I understand the data segment "lives" below the program break, hence
>> it is a way of describing the range from which the user can obtain a valid
>> tagged pointer.>
>> Said that, I am not really sure on how do you want me to document this (my aim
>> is for this to be clear to the userspace developers). Could you please propose
>> something?
>
> [...], it is in the memory ranges privately owned by a
> userspace process and it is obtained in one of the
> following ways:
>
> - mmap done by the process itself, [...]
>
> - brk syscall done by the process itself.
> (i.e. the heap area between the initial location
> of the program break at process creation and its
> current location.)
>
> - any memory mapped by the kernel [...]
>
> the data segment that's part of the process image is
> already covered by the last point.
>
Thanks Szabolcs, I will update the document accordingly.
--
Regards,
Vincenzo
^ permalink raw reply
* [PATCH] dt: leds-lm36274.txt: fix a broken reference to ti-lmu.txt
From: Mauro Carvalho Chehab @ 2019-06-13 10:23 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Jacek Anaszewski, Pavel Machek, Dan Murphy,
Rob Herring, Mark Rutland, linux-leds, devicetree
There's a typo there:
ti_lmu.txt -> ti-lmu.txt
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/devicetree/bindings/leds/leds-lm36274.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/leds/leds-lm36274.txt b/Documentation/devicetree/bindings/leds/leds-lm36274.txt
index 456a589c65f0..39c230d59a4d 100644
--- a/Documentation/devicetree/bindings/leds/leds-lm36274.txt
+++ b/Documentation/devicetree/bindings/leds/leds-lm36274.txt
@@ -6,7 +6,7 @@ up to 29V total output voltage. The 11-bit LED current is programmable via
the I2C bus and/or controlled via a logic level PWM input from 60 uA to 30 mA.
Parent device properties are documented in
-Documentation/devicetree/bindings/mfd/ti_lmu.txt
+Documentation/devicetree/bindings/mfd/ti-lmu.txt
Regulator properties are documented in
Documentation/devicetree/bindings/regulator/lm363x-regulator.txt
--
2.21.0
^ permalink raw reply related
* [PATCH] scripts/documentation-file-ref-check: ignore output dir
From: Mauro Carvalho Chehab @ 2019-06-13 10:29 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet
When there's no Documentation/output directory, the script will
complain about those missing references:
Documentation/doc-guide/sphinx.rst: Documentation/output
Documentation/doc-guide/sphinx.rst: Documentation/output
Documentation/process/howto.rst: Documentation/output
Documentation/translations/it_IT/doc-guide/sphinx.rst: Documentation/output
Documentation/translations/it_IT/doc-guide/sphinx.rst: Documentation/output
Documentation/translations/it_IT/process/howto.rst: Documentation/output
Documentation/translations/ja_JP/howto.rst: Documentation/output
Documentation/translations/ko_KR/howto.rst: Documentation/output
Those are false positives, so add an ignore rule for them.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
scripts/documentation-file-ref-check | 3 +++
1 file changed, 3 insertions(+)
diff --git a/scripts/documentation-file-ref-check b/scripts/documentation-file-ref-check
index a4139a576726..7784c54aa38b 100755
--- a/scripts/documentation-file-ref-check
+++ b/scripts/documentation-file-ref-check
@@ -90,6 +90,9 @@ while (<IN>) {
# Skip this script
next if ($f eq $scriptname);
+ # Ignore the dir where documentation will be built
+ next if ($ln =~ m,\b(\S*)Documentation/output,);
+
if ($ln =~ m,\b(\S*)(Documentation/[A-Za-z0-9\_\.\,\~/\*\[\]\?+-]*)(.*),) {
my $prefix = $1;
my $ref = $2;
--
2.21.0
^ permalink raw reply related
* [RFC 0/7] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, tee-dev, Sumit Garg
Add support for TEE based trusted keys where TEE provides the functionality
to seal and unseal trusted keys using hardware unique key. Also, this is
an alternative in case platform doesn't possess a TPM device.
This series also adds some TEE features like:
Patch #1, #2 enables support for registered kernel shared memory with TEE.
Patch #3 enables support for private kernel login method required for
cases like trusted keys where we don't wan't user-space to directly access
TEE service to retrieve trusted key contents.
Rest of the patches from #4 to #7 adds support for TEE based trusted keys.
This patch-set has been tested with OP-TEE based pseudo TA which can be
found here [1].
Looking forward to your valuable feedback/suggestions.
[1] https://github.com/OP-TEE/optee_os/pull/3082
Sumit Garg (7):
tee: optee: allow kernel pages to register as shm
tee: enable support to register kernel memory
tee: add private login method for kernel clients
KEYS: trusted: Introduce TEE based Trusted Keys
KEYS: encrypted: Allow TEE based trusted master keys
doc: keys: Document usage of TEE based Trusted Keys
MAINTAINERS: Add entry for TEE based Trusted Keys
Documentation/security/keys/tee-trusted.rst | 93 +++++
MAINTAINERS | 9 +
drivers/tee/optee/call.c | 7 +
drivers/tee/tee_core.c | 6 +
drivers/tee/tee_shm.c | 16 +-
include/keys/tee_trusted.h | 84 ++++
include/keys/trusted-type.h | 1 +
include/linux/tee_drv.h | 1 +
include/uapi/linux/tee.h | 2 +
security/keys/Kconfig | 3 +
security/keys/Makefile | 3 +
security/keys/encrypted-keys/masterkey_trusted.c | 10 +-
security/keys/tee_trusted.c | 506 +++++++++++++++++++++++
13 files changed, 737 insertions(+), 4 deletions(-)
create mode 100644 Documentation/security/keys/tee-trusted.rst
create mode 100644 include/keys/tee_trusted.h
create mode 100644 security/keys/tee_trusted.c
--
2.7.4
^ permalink raw reply
* [RFC 1/7] tee: optee: allow kernel pages to register as shm
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>
Kernel pages are marked as normal type memory only so allow kernel pages
to be registered as shared memory with OP-TEE.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
drivers/tee/optee/call.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index aa94270..bce45b1 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -553,6 +553,13 @@ static int check_mem_type(unsigned long start, size_t num_pages)
struct mm_struct *mm = current->mm;
int rc;
+ /*
+ * Allow kernel address to register with OP-TEE as kernel
+ * pages are configured as normal memory only.
+ */
+ if (virt_addr_valid(start))
+ return 0;
+
down_read(&mm->mmap_sem);
rc = __check_mem_type(find_vma(mm, start),
start + num_pages * PAGE_SIZE);
--
2.7.4
^ permalink raw reply related
* [RFC 2/7] tee: enable support to register kernel memory
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>
Enable support to register kernel memory reference with TEE. This change
will allow TEE bus drivers to register memory references.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
drivers/tee/tee_shm.c | 16 ++++++++++++++--
include/linux/tee_drv.h | 1 +
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 2da026f..5c69b89 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -9,6 +9,7 @@
#include <linux/sched.h>
#include <linux/slab.h>
#include <linux/tee_drv.h>
+#include <linux/uio.h>
#include "tee_private.h"
static void tee_shm_release(struct tee_shm *shm)
@@ -224,13 +225,14 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
{
struct tee_device *teedev = ctx->teedev;
const u32 req_flags = TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED;
+ const u32 req_ker_flags = TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED;
struct tee_shm *shm;
void *ret;
int rc;
int num_pages;
unsigned long start;
- if (flags != req_flags)
+ if (flags != req_flags && flags != req_ker_flags)
return ERR_PTR(-ENOTSUPP);
if (!tee_device_get(teedev))
@@ -264,7 +266,17 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
goto err;
}
- rc = get_user_pages_fast(start, num_pages, FOLL_WRITE, shm->pages);
+ if (flags & TEE_SHM_USER_MAPPED) {
+ rc = get_user_pages_fast(start, num_pages, FOLL_WRITE,
+ shm->pages);
+ } else {
+ const struct kvec kiov = {
+ .iov_base = (void *)start,
+ .iov_len = PAGE_SIZE
+ };
+
+ rc = get_kernel_pages(&kiov, num_pages, 0, shm->pages);
+ }
if (rc > 0)
shm->num_pages = rc;
if (rc != num_pages) {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 7a03f68..dedf8fa 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -26,6 +26,7 @@
#define TEE_SHM_REGISTER BIT(3) /* Memory registered in secure world */
#define TEE_SHM_USER_MAPPED BIT(4) /* Memory mapped in user space */
#define TEE_SHM_POOL BIT(5) /* Memory allocated from pool */
+#define TEE_SHM_KERNEL_MAPPED BIT(6) /* Memory mapped in kernel space */
struct device;
struct tee_device;
--
2.7.4
^ permalink raw reply related
* [RFC 3/7] tee: add private login method for kernel clients
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>
There are use-cases where user-space shouldn't be allowed to communicate
directly with a TEE device which is dedicated to provide a specific
service for a kernel client. So add a private login method for kernel
clients and disallow user-space to open-session using this login method.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
drivers/tee/tee_core.c | 6 ++++++
include/uapi/linux/tee.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 0f16d9f..4581bd1 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -334,6 +334,12 @@ static int tee_ioctl_open_session(struct tee_context *ctx,
goto out;
}
+ if (arg.clnt_login == TEE_IOCTL_LOGIN_REE_KERNEL) {
+ pr_err("login method not allowed for user-space client\n");
+ rc = -EPERM;
+ goto out;
+ }
+
rc = ctx->teedev->desc->ops->open_session(ctx, &arg, params);
if (rc)
goto out;
diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
index 4b9eb06..f33c69c 100644
--- a/include/uapi/linux/tee.h
+++ b/include/uapi/linux/tee.h
@@ -172,6 +172,8 @@ struct tee_ioctl_buf_data {
#define TEE_IOCTL_LOGIN_APPLICATION 4
#define TEE_IOCTL_LOGIN_USER_APPLICATION 5
#define TEE_IOCTL_LOGIN_GROUP_APPLICATION 6
+/* Private login method for REE kernel clients */
+#define TEE_IOCTL_LOGIN_REE_KERNEL 0x80000000
/**
* struct tee_ioctl_param - parameter
--
2.7.4
^ permalink raw reply related
* [RFC 4/7] KEYS: trusted: Introduce TEE based Trusted Keys
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>
Add support for TEE based trusted keys where TEE provides the functionality
to seal and unseal trusted keys using hardware unique key.
Refer to Documentation/tee.txt for detailed information about TEE.
Approach taken in this patch acts as an alternative to a TPM device in case
platform doesn't possess one.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
include/keys/tee_trusted.h | 84 ++++++++
include/keys/trusted-type.h | 1 +
security/keys/Kconfig | 3 +
security/keys/Makefile | 3 +
security/keys/tee_trusted.c | 506 ++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 597 insertions(+)
create mode 100644 include/keys/tee_trusted.h
create mode 100644 security/keys/tee_trusted.c
diff --git a/include/keys/tee_trusted.h b/include/keys/tee_trusted.h
new file mode 100644
index 0000000..e5c0042
--- /dev/null
+++ b/include/keys/tee_trusted.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 Linaro Ltd.
+ *
+ * Author:
+ * Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#ifndef __TEE_TRUSTED_KEY_H
+#define __TEE_TRUSTED_KEY_H
+
+#include <linux/tee_drv.h>
+
+#define DRIVER_NAME "tee-trusted-key"
+
+/*
+ * Get random data for symmetric key
+ *
+ * [out] memref[0] Random data
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_GET_RANDOM 0x0
+
+/*
+ * Seal trusted key using hardware unique key
+ *
+ * [in] memref[0] Plain key
+ * [out] memref[1] Sealed key datablob
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_SEAL 0x1
+
+/*
+ * Unseal trusted key using hardware unique key
+ *
+ * [in] memref[0] Sealed key datablob
+ * [out] memref[1] Plain key
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_UNSEAL 0x2
+
+/**
+ * struct trusted_key_private - TEE Trusted key private data
+ * @dev: TEE based Trusted key device.
+ * @ctx: TEE context handler.
+ * @session_id: Trusted key TA session identifier.
+ * @shm_pool: Memory pool shared with TEE device.
+ */
+struct trusted_key_private {
+ struct device *dev;
+ struct tee_context *ctx;
+ u32 session_id;
+ u32 data_rate;
+ struct tee_shm *shm_pool;
+};
+
+#define TEE_KEY_DEBUG 0
+
+#if TEE_KEY_DEBUG
+static inline void dump_tee_payload(struct trusted_key_payload *p)
+{
+ pr_info("trusted_key: key_len %d\n", p->key_len);
+ print_hex_dump(KERN_INFO, "key ", DUMP_PREFIX_NONE,
+ 16, 1, p->key, p->key_len, 0);
+ pr_info("trusted_key: bloblen %d\n", p->blob_len);
+ print_hex_dump(KERN_INFO, "blob ", DUMP_PREFIX_NONE,
+ 16, 1, p->blob, p->blob_len, 0);
+}
+#else
+static inline void dump_tee_payload(struct trusted_key_payload *p)
+{
+}
+#endif
+
+#endif
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index a94c03a..363ec83 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -41,5 +41,6 @@ struct trusted_key_options {
};
extern struct key_type key_type_trusted;
+extern struct key_type key_type_tee_trusted;
#endif /* _KEYS_TRUSTED_TYPE_H */
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index ee502e4..b206a20 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -70,6 +70,9 @@ config TRUSTED_KEYS
if the boot PCRs and other criteria match. Userspace will only ever
see encrypted blobs.
+ It also provides support for alternative TEE based Trusted keys
+ generation and sealing in case TPM isn't present.
+
If you are unsure as to whether this is required, answer N.
config ENCRYPTED_KEYS
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 9cef540..07ad3e2 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -30,3 +30,6 @@ obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += keyctl_pkey.o
obj-$(CONFIG_BIG_KEYS) += big_key.o
obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys/
+ifdef CONFIG_TEE
+obj-$(CONFIG_TRUSTED_KEYS) += tee_trusted.o
+endif
diff --git a/security/keys/tee_trusted.c b/security/keys/tee_trusted.c
new file mode 100644
index 0000000..081e45e
--- /dev/null
+++ b/security/keys/tee_trusted.c
@@ -0,0 +1,506 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Linaro Ltd.
+ *
+ * Author:
+ * Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#include <linux/err.h>
+#include <linux/key-type.h>
+#include <linux/module.h>
+#include <linux/parser.h>
+#include <linux/rcupdate.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/tpm.h>
+#include <linux/uaccess.h>
+#include <linux/uuid.h>
+
+#include <keys/trusted-type.h>
+#include <keys/user-type.h>
+#include <keys/tee_trusted.h>
+
+static struct trusted_key_private pvt_data;
+
+/*
+ * Have the TEE seal(encrypt) the symmetric key
+ */
+static int tee_key_seal(struct trusted_key_payload *p)
+{
+ int ret = 0;
+ struct tee_ioctl_invoke_arg inv_arg;
+ struct tee_param param[4];
+ struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
+
+ memset(&inv_arg, 0, sizeof(inv_arg));
+ memset(¶m, 0, sizeof(param));
+
+ reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
+ p->key_len, TEE_SHM_DMA_BUF |
+ TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm_in)) {
+ dev_err(pvt_data.dev, "key shm register failed\n");
+ return PTR_ERR(reg_shm_in);
+ }
+
+ reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
+ sizeof(p->blob), TEE_SHM_DMA_BUF |
+ TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm_out)) {
+ dev_err(pvt_data.dev, "blob shm register failed\n");
+ ret = PTR_ERR(reg_shm_out);
+ goto out;
+ }
+
+ inv_arg.func = TA_CMD_SEAL;
+ inv_arg.session = pvt_data.session_id;
+ inv_arg.num_params = 4;
+
+ param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+ param[0].u.memref.shm = reg_shm_in;
+ param[0].u.memref.size = p->key_len;
+ param[0].u.memref.shm_offs = 0;
+ param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+ param[1].u.memref.shm = reg_shm_out;
+ param[1].u.memref.size = sizeof(p->blob);
+ param[1].u.memref.shm_offs = 0;
+
+ ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+ if ((ret < 0) || (inv_arg.ret != 0)) {
+ dev_err(pvt_data.dev, "TA_CMD_SEAL invoke err: %x\n",
+ inv_arg.ret);
+ ret = -EFAULT;
+ } else {
+ p->blob_len = param[1].u.memref.size;
+ }
+
+out:
+ if (reg_shm_out)
+ tee_shm_free(reg_shm_out);
+ if (reg_shm_in)
+ tee_shm_free(reg_shm_in);
+
+ return ret;
+}
+
+/*
+ * Have the TEE unseal(decrypt) the symmetric key
+ */
+static int tee_key_unseal(struct trusted_key_payload *p)
+{
+ int ret = 0;
+ struct tee_ioctl_invoke_arg inv_arg;
+ struct tee_param param[4];
+ struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
+
+ memset(&inv_arg, 0, sizeof(inv_arg));
+ memset(¶m, 0, sizeof(param));
+
+ reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
+ p->blob_len, TEE_SHM_DMA_BUF |
+ TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm_in)) {
+ dev_err(pvt_data.dev, "blob shm register failed\n");
+ return PTR_ERR(reg_shm_in);
+ }
+
+ reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
+ sizeof(p->key), TEE_SHM_DMA_BUF |
+ TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm_out)) {
+ dev_err(pvt_data.dev, "key shm register failed\n");
+ ret = PTR_ERR(reg_shm_out);
+ goto out;
+ }
+
+ inv_arg.func = TA_CMD_UNSEAL;
+ inv_arg.session = pvt_data.session_id;
+ inv_arg.num_params = 4;
+
+ param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+ param[0].u.memref.shm = reg_shm_in;
+ param[0].u.memref.size = p->blob_len;
+ param[0].u.memref.shm_offs = 0;
+ param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+ param[1].u.memref.shm = reg_shm_out;
+ param[1].u.memref.size = sizeof(p->key);
+ param[1].u.memref.shm_offs = 0;
+
+ ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+ if ((ret < 0) || (inv_arg.ret != 0)) {
+ dev_err(pvt_data.dev, "TA_CMD_UNSEAL invoke err: %x\n",
+ inv_arg.ret);
+ ret = -EFAULT;
+ } else {
+ p->key_len = param[1].u.memref.size;
+ }
+
+out:
+ if (reg_shm_out)
+ tee_shm_free(reg_shm_out);
+ if (reg_shm_in)
+ tee_shm_free(reg_shm_in);
+
+ return ret;
+}
+
+/*
+ * Have the TEE generate random symmetric key
+ */
+static int tee_get_random(unsigned char *key, unsigned int key_len)
+{
+ int ret = 0;
+ struct tee_ioctl_invoke_arg inv_arg;
+ struct tee_param param[4];
+ struct tee_shm *reg_shm = NULL;
+
+ memset(&inv_arg, 0, sizeof(inv_arg));
+ memset(¶m, 0, sizeof(param));
+
+ reg_shm = tee_shm_register(pvt_data.ctx, (unsigned long)key, key_len,
+ TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm)) {
+ dev_err(pvt_data.dev, "random key shm register failed\n");
+ return PTR_ERR(reg_shm);
+ }
+
+ inv_arg.func = TA_CMD_GET_RANDOM;
+ inv_arg.session = pvt_data.session_id;
+ inv_arg.num_params = 4;
+
+ param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+ param[0].u.memref.shm = reg_shm;
+ param[0].u.memref.size = key_len;
+ param[0].u.memref.shm_offs = 0;
+
+ ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+ if ((ret < 0) || (inv_arg.ret != 0)) {
+ dev_err(pvt_data.dev, "TA_CMD_GET_RANDOM invoke err: %x\n",
+ inv_arg.ret);
+ ret = -EFAULT;
+ } else {
+ ret = param[0].u.memref.size;
+ }
+
+ tee_shm_free(reg_shm);
+
+ return ret;
+}
+
+enum {
+ Opt_err,
+ Opt_new, Opt_load
+};
+
+static const match_table_t key_tokens = {
+ {Opt_new, "new"},
+ {Opt_load, "load"},
+ {Opt_err, NULL}
+};
+
+/*
+ * datablob_parse - parse the keyctl data and fill in the
+ * payload structure
+ *
+ * On success returns 0, otherwise -EINVAL.
+ */
+static int datablob_parse(char *datablob, struct trusted_key_payload *p)
+{
+ substring_t args[MAX_OPT_ARGS];
+ long keylen;
+ int ret = -EINVAL;
+ int key_cmd;
+ char *c;
+
+ /* main command */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+
+ key_cmd = match_token(c, key_tokens, args);
+ switch (key_cmd) {
+ case Opt_new:
+ /* first argument is key size */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+ ret = kstrtol(c, 10, &keylen);
+ if (ret < 0 || keylen < MIN_KEY_SIZE || keylen > MAX_KEY_SIZE)
+ return -EINVAL;
+ p->key_len = keylen;
+ ret = Opt_new;
+ break;
+ case Opt_load:
+ /* first argument is sealed blob */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+ p->blob_len = strlen(c) / 2;
+ if (p->blob_len > MAX_BLOB_SIZE)
+ return -EINVAL;
+ ret = hex2bin(p->blob, c, p->blob_len);
+ if (ret < 0)
+ return -EINVAL;
+ ret = Opt_load;
+ break;
+ case Opt_err:
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
+{
+ struct trusted_key_payload *p = NULL;
+ int ret;
+
+ ret = key_payload_reserve(key, sizeof(*p));
+ if (ret < 0)
+ return p;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+
+ return p;
+}
+
+/*
+ * trusted_instantiate - create a new trusted key
+ *
+ * Unseal an existing trusted blob or, for a new key, get a
+ * random key, then seal and create a trusted key-type key,
+ * adding it to the specified keyring.
+ *
+ * On success, return 0. Otherwise return errno.
+ */
+static int trusted_instantiate(struct key *key,
+ struct key_preparsed_payload *prep)
+{
+ struct trusted_key_payload *payload = NULL;
+ size_t datalen = prep->datalen;
+ char *datablob;
+ int ret = 0;
+ int key_cmd;
+ size_t key_len;
+
+ if (datalen <= 0 || datalen > 32767 || !prep->data)
+ return -EINVAL;
+
+ datablob = kmalloc(datalen + 1, GFP_KERNEL);
+ if (!datablob)
+ return -ENOMEM;
+ memcpy(datablob, prep->data, datalen);
+ datablob[datalen] = '\0';
+
+ payload = trusted_payload_alloc(key);
+ if (!payload) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ key_cmd = datablob_parse(datablob, payload);
+ if (key_cmd < 0) {
+ ret = key_cmd;
+ goto out;
+ }
+
+ dump_tee_payload(payload);
+
+ switch (key_cmd) {
+ case Opt_load:
+ ret = tee_key_unseal(payload);
+ dump_tee_payload(payload);
+ if (ret < 0)
+ dev_err(pvt_data.dev, "key_unseal failed (%d)\n", ret);
+ break;
+ case Opt_new:
+ key_len = payload->key_len;
+ ret = tee_get_random(payload->key, key_len);
+ if (ret != key_len) {
+ dev_err(pvt_data.dev, "key_create failed (%d)\n", ret);
+ goto out;
+ }
+
+ ret = tee_key_seal(payload);
+ if (ret < 0)
+ dev_err(pvt_data.dev, "key_seal failed (%d)\n", ret);
+ dump_tee_payload(payload);
+ break;
+ default:
+ ret = -EINVAL;
+ goto out;
+ }
+out:
+ kzfree(datablob);
+ if (!ret)
+ rcu_assign_keypointer(key, payload);
+ else
+ kzfree(payload);
+ return ret;
+}
+
+static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
+{
+ dev_info(pvt_data.dev, "trusted key update method not supported\n");
+
+ return -EINVAL;
+}
+
+/*
+ * trusted_read - copy the sealed blob data to userspace in hex.
+ * On success, return to userspace the trusted key datablob size.
+ */
+static long trusted_read(const struct key *key, char __user *buffer,
+ size_t buflen)
+{
+ const struct trusted_key_payload *p;
+ char *ascii_buf;
+ char *bufp;
+ int i;
+
+ p = dereference_key_locked(key);
+ if (!p)
+ return -EINVAL;
+
+ if (buffer && buflen >= 2 * p->blob_len) {
+ ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
+ if (!ascii_buf)
+ return -ENOMEM;
+
+ bufp = ascii_buf;
+ for (i = 0; i < p->blob_len; i++)
+ bufp = hex_byte_pack(bufp, p->blob[i]);
+ if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
+ kzfree(ascii_buf);
+ return -EFAULT;
+ }
+ kzfree(ascii_buf);
+ }
+ return 2 * p->blob_len;
+}
+
+/*
+ * trusted_destroy - clear and free the key's payload
+ */
+static void trusted_destroy(struct key *key)
+{
+ kzfree(key->payload.data[0]);
+}
+
+struct key_type key_type_tee_trusted = {
+ .name = "trusted",
+ .instantiate = trusted_instantiate,
+ .update = trusted_update,
+ .destroy = trusted_destroy,
+ .describe = user_describe,
+ .read = trusted_read,
+};
+EXPORT_SYMBOL_GPL(key_type_tee_trusted);
+
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+ if (ver->impl_id == TEE_IMPL_ID_OPTEE)
+ return 1;
+ else
+ return 0;
+}
+
+static int trusted_key_probe(struct device *dev)
+{
+ struct tee_client_device *rng_device = to_tee_client_device(dev);
+ int ret = 0, err = -ENODEV;
+ struct tee_ioctl_open_session_arg sess_arg;
+
+ memset(&sess_arg, 0, sizeof(sess_arg));
+
+ /* Open context with TEE driver */
+ pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
+ NULL);
+ if (IS_ERR(pvt_data.ctx))
+ return -ENODEV;
+
+ /* Open session with hwrng Trusted App */
+ memcpy(sess_arg.uuid, rng_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
+ sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+ sess_arg.num_params = 0;
+
+ ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);
+ if ((ret < 0) || (sess_arg.ret != 0)) {
+ dev_err(dev, "tee_client_open_session failed, err: %x\n",
+ sess_arg.ret);
+ err = -EINVAL;
+ goto out_ctx;
+ }
+ pvt_data.session_id = sess_arg.session;
+
+ ret = register_key_type(&key_type_tee_trusted);
+ if (ret < 0)
+ goto out_sess;
+
+ pvt_data.dev = dev;
+
+ return 0;
+
+out_sess:
+ tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+out_ctx:
+ tee_client_close_context(pvt_data.ctx);
+
+ return err;
+}
+
+static int trusted_key_remove(struct device *dev)
+{
+ unregister_key_type(&key_type_tee_trusted);
+ tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+ tee_client_close_context(pvt_data.ctx);
+
+ return 0;
+}
+
+static const struct tee_client_device_id trusted_key_id_table[] = {
+ {UUID_INIT(0xf04a0fe7, 0x1f5d, 0x4b9b,
+ 0xab, 0xf7, 0x61, 0x9b, 0x85, 0xb4, 0xce, 0x8c)},
+ {}
+};
+
+MODULE_DEVICE_TABLE(tee, trusted_key_id_table);
+
+static struct tee_client_driver trusted_key_driver = {
+ .id_table = trusted_key_id_table,
+ .driver = {
+ .name = DRIVER_NAME,
+ .bus = &tee_bus_type,
+ .probe = trusted_key_probe,
+ .remove = trusted_key_remove,
+ },
+};
+
+static int __init init_tee_trusted(void)
+{
+ struct tpm_chip *chip;
+
+ /*
+ * Check for TPM availability as that is default source for trusted
+ * keys. If not present, then register driver for TEE based device
+ * providing support for trusted keys.
+ */
+ chip = tpm_default_chip();
+ if (chip)
+ return 0;
+
+ return driver_register(&trusted_key_driver.driver);
+}
+
+static void __exit cleanup_tee_trusted(void)
+{
+ driver_unregister(&trusted_key_driver.driver);
+}
+
+late_initcall(init_tee_trusted);
+module_exit(cleanup_tee_trusted);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Sumit Garg <sumit.garg@linaro.org>");
+MODULE_DESCRIPTION("TEE based trusted keys");
--
2.7.4
^ permalink raw reply related
* [RFC 5/7] KEYS: encrypted: Allow TEE based trusted master keys
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>
Allow search for TEE based trusted keys to act as master keys in case
TPM device is not present.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
security/keys/encrypted-keys/masterkey_trusted.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
index c68528a..cfac27f 100644
--- a/security/keys/encrypted-keys/masterkey_trusted.c
+++ b/security/keys/encrypted-keys/masterkey_trusted.c
@@ -23,6 +23,9 @@
* Trusted keys are sealed to PCRs and other metadata. Although userspace
* manages both trusted/encrypted key-types, like the encrypted key type
* data, trusted key type data is not visible decrypted from userspace.
+ *
+ * Also, check for alternate trusted keys provided via TEE in case there
+ * is no TPM available.
*/
struct key *request_trusted_key(const char *trusted_desc,
const u8 **master_key, size_t *master_keylen)
@@ -31,8 +34,11 @@ struct key *request_trusted_key(const char *trusted_desc,
struct key *tkey;
tkey = request_key(&key_type_trusted, trusted_desc, NULL);
- if (IS_ERR(tkey))
- goto error;
+ if (IS_ERR(tkey)) {
+ tkey = request_key(&key_type_tee_trusted, trusted_desc, NULL);
+ if (IS_ERR(tkey))
+ goto error;
+ }
down_read(&tkey->sem);
tpayload = tkey->payload.data[0];
--
2.7.4
^ permalink raw reply related
* Re: [RFC 6/7] doc: keys: Document usage of TEE based Trusted Keys
From: Jarkko Sakkinen @ 2019-06-13 15:34 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, jens.wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, ard.biesheuvel,
daniel.thompson, linux-doc, linux-kernel, tee-dev
In-Reply-To: <1560421833-27414-7-git-send-email-sumit.garg@linaro.org>
On Thu, Jun 13, 2019 at 04:00:32PM +0530, Sumit Garg wrote:
> Provide documentation for usage of TEE based Trusted Keys via existing
> user-space "keyctl" utility. Also, document various use-cases.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Sorry missed this patch. Anyway, I don't think we want multiple trusted
keys subsystems. You have to fix the existing one if you care to get
these changes in. There is no really other way around this.
/Jarkko
^ permalink raw reply
* [RFC 6/7] doc: keys: Document usage of TEE based Trusted Keys
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>
Provide documentation for usage of TEE based Trusted Keys via existing
user-space "keyctl" utility. Also, document various use-cases.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
Documentation/security/keys/tee-trusted.rst | 93 +++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
create mode 100644 Documentation/security/keys/tee-trusted.rst
diff --git a/Documentation/security/keys/tee-trusted.rst b/Documentation/security/keys/tee-trusted.rst
new file mode 100644
index 0000000..ef03745
--- /dev/null
+++ b/Documentation/security/keys/tee-trusted.rst
@@ -0,0 +1,93 @@
+======================
+TEE based Trusted Keys
+======================
+
+TEE based Trusted Keys provides an alternative approach for providing Trusted
+Keys in case TPM chip isn't present.
+
+Trusted Keys use a TEE service/device both to generate and to seal the keys.
+Keys are sealed under a hardware unique key in the TEE, and only unsealed by
+the TEE.
+
+For more information about TEE, refer to ``Documentation/tee.txt``.
+
+Usage::
+
+ keyctl add trusted name "new keylen" ring
+ keyctl add trusted name "load hex_blob" ring
+ keyctl print keyid
+
+"keyctl print" returns an ascii hex copy of the sealed key, which is in format
+specific to TEE device implementation. The key length for new keys are always
+in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
+
+Examples of trusted key and its usage as 'master' key for encrypted key usage:
+
+More details about encrypted keys can be found here:
+``Documentation/security/keys/trusted-encrypted.rst``
+
+Create and save a trusted key named "kmk" of length 32 bytes::
+
+ $ keyctl add trusted kmk "new 32" @u
+ 754414669
+
+ $ keyctl show
+ Session Keyring
+ 827385718 --alswrv 0 65534 keyring: _uid_ses.0
+ 274124851 --alswrv 0 65534 \_ keyring: _uid.0
+ 754414669 --als-rv 0 0 \_ trusted: kmk
+
+ $ keyctl print 754414669
+ 15676790697861b422175596ae001c2f505cea2c6f3ebbc5fb08eeb1f343a07e
+
+ $ keyctl pipe 754414669 > kmk.blob
+
+Load a trusted key from the saved blob::
+
+ $ keyctl add trusted kmk "load `cat kmk.blob`" @u
+ 491638700
+
+ $ keyctl print 491638700
+ 15676790697861b422175596ae001c2f505cea2c6f3ebbc5fb08eeb1f343a07e
+
+The initial consumer of trusted keys is EVM, which at boot time needs a high
+quality symmetric key for HMAC protection of file metadata. The use of a
+TEE based trusted key provides security that the EVM key has not been
+compromised by a user level problem and tied to particular hardware.
+
+Create and save an encrypted key "evm" using the above trusted key "kmk":
+
+option 1: omitting 'format'::
+
+ $ keyctl add encrypted evm "new trusted:kmk 32" @u
+ 608915065
+
+option 2: explicitly defining 'format' as 'default'::
+
+ $ keyctl add encrypted evm "new default trusted:kmk 32" @u
+ 608915065
+
+ $ keyctl print 608915065
+ default trusted:kmk 32 f380ac588a925f488d5be007cf23e4c900b8b652ab62241c8
+ ed54906189b6659d139d619d4b51752a2645537b11fd44673f13154a65b3f595d5fb2131
+ 2fe45529ea0407c644ea4026f2a1a75661f2c9b66
+
+ $ keyctl pipe 608915065 > evm.blob
+
+Load an encrypted key "evm" from saved blob::
+
+ $ keyctl add encrypted evm "load `cat evm.blob`" @u
+ 831684262
+
+ $ keyctl print 831684262
+ default trusted:kmk 32 f380ac588a925f488d5be007cf23e4c900b8b652ab62241c8
+ ed54906189b6659d139d619d4b51752a2645537b11fd44673f13154a65b3f595d5fb2131
+ 2fe45529ea0407c644ea4026f2a1a75661f2c9b66
+
+Other uses for trusted and encrypted keys, such as for disk and file encryption
+are anticipated. In particular the 'ecryptfs' encrypted keys format can be used
+to mount an eCryptfs filesystem. More details about the usage can be found in
+the file ``Documentation/security/keys/ecryptfs.rst``.
+
+Another format 'enc32' can be used to support encrypted keys with payload size
+of 32 bytes.
--
2.7.4
^ permalink raw reply related
* [RFC 7/7] MAINTAINERS: Add entry for TEE based Trusted Keys
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>
Add MAINTAINERS entry for TEE based Trusted Keys framework.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 57f496c..db84fc4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8728,6 +8728,15 @@ F: include/keys/trusted-type.h
F: security/keys/trusted.c
F: security/keys/trusted.h
+KEYS-TEE-TRUSTED
+M: Sumit Garg <sumit.garg@linaro.org>
+L: linux-integrity@vger.kernel.org
+L: keyrings@vger.kernel.org
+S: Supported
+F: Documentation/security/keys/tee-trusted.rst
+F: include/keys/tee_trusted.h
+F: security/keys/tee_trusted.c
+
KEYS/KEYRINGS:
M: David Howells <dhowells@redhat.com>
L: keyrings@vger.kernel.org
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Szabolcs Nagy @ 2019-06-13 15:32 UTC (permalink / raw)
To: Vincenzo Frascino, Catalin Marinas
Cc: nd, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, linux-mm@kvack.org,
linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, Will Deacon, Andrey Konovalov,
Alexander Viro
In-Reply-To: <5963d144-be9b-78d8-9130-ef92bc66b1fd@arm.com>
On 13/06/2019 15:03, Vincenzo Frascino wrote:
> On 13/06/2019 13:28, Szabolcs Nagy wrote:
>> On 13/06/2019 12:16, Vincenzo Frascino wrote:
>>> On 13/06/2019 11:14, Szabolcs Nagy wrote:
>>>> On 13/06/2019 10:20, Catalin Marinas wrote:
>>>>> On Wed, Jun 12, 2019 at 05:30:34PM +0100, Szabolcs Nagy wrote:
>>>>>> On 12/06/2019 15:21, Vincenzo Frascino wrote:
>>>>>>> + - a mapping below sbrk(0) done by the process itself
>>>>>>
>>>>>> doesn't the mmap rule cover this?
>>>>>
>>>>> IIUC it doesn't cover it as that's memory mapped by the kernel
>>>>> automatically on access vs a pointer returned by mmap(). The statement
>>>>> above talks about how the address is obtained by the user.
>>>>
>>>> ok i read 'mapping below sbrk' as an mmap (possibly MAP_FIXED)
>>>> that happens to be below the heap area.
>>>>
>>>> i think "below sbrk(0)" is not the best term to use: there
>>>> may be address range below the heap area that can be mmapped
>>>> and thus below sbrk(0) and sbrk is a posix api not a linux
>>>> syscall, the libc can implement it with mmap or whatever.
>>>>
>>>> i'm not sure what the right term for 'heap area' is
>>>> (the address range between syscall(__NR_brk,0) at
>>>> program startup and its current value?)
>>>>
>>>
>>> I used sbrk(0) with the meaning of "end of the process's data segment" not
>>> implying that this is a syscall, but just as a useful way to identify the mapping.
>>> I agree that it is a posix function implemented by libc but when it is used with
>>> 0 finds the current location of the program break, which can be changed by brk()
>>> and depending on the new address passed to this syscall can have the effect of
>>> allocating or deallocating memory.
>>>
>>> Will changing sbrk(0) with "end of the process's data segment" make it more clear?
>>
>> i don't understand what's the relevance of the *end*
>> of the data segment.
>>
>> i'd expect the text to say something about the address
>> range of the data segment.
>>
>> i can do
>>
>> mmap((void*)65536, 65536, PROT_READ|PROT_WRITE, MAP_FIXED|MAP_SHARED|MAP_ANON, -1, 0);
>>
>> and it will be below the end of the data segment.
>>
>
> As far as I understand the data segment "lives" below the program break, hence
> it is a way of describing the range from which the user can obtain a valid
> tagged pointer.>
> Said that, I am not really sure on how do you want me to document this (my aim
> is for this to be clear to the userspace developers). Could you please propose
> something?
[...], it is in the memory ranges privately owned by a
userspace process and it is obtained in one of the
following ways:
- mmap done by the process itself, [...]
- brk syscall done by the process itself.
(i.e. the heap area between the initial location
of the program break at process creation and its
current location.)
- any memory mapped by the kernel [...]
the data segment that's part of the process image is
already covered by the last point.
^ permalink raw reply
* Re: [PATCH v3 1/3] lib/test_kasan: Add bitops tests
From: Andrey Ryabinin @ 2019-06-13 10:49 UTC (permalink / raw)
To: Marco Elver, peterz, dvyukov, glider, andreyknvl, mark.rutland,
hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev
In-Reply-To: <20190531150828.157832-2-elver@google.com>
On 5/31/19 6:08 PM, Marco Elver wrote:
> This adds bitops tests to the test_kasan module. In a follow-up patch,
> support for bitops instrumentation will be added.
>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> Changes in v3:
> * Use kzalloc instead of kmalloc.
> * Use sizeof(*bits).
>
> Changes in v2:
> * Use BITS_PER_LONG.
> * Use heap allocated memory for test, as newer compilers (correctly)
> warn on OOB stack access.
> ---
> lib/test_kasan.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 7de2702621dc..1ef9702327d2 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -11,16 +11,17 @@
>
> #define pr_fmt(fmt) "kasan test: %s " fmt, __func__
>
> +#include <linux/bitops.h>
> #include <linux/delay.h>
> +#include <linux/kasan.h>
> #include <linux/kernel.h>
> -#include <linux/mman.h>
> #include <linux/mm.h>
> +#include <linux/mman.h>
> +#include <linux/module.h>
> #include <linux/printk.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/uaccess.h>
> -#include <linux/module.h>
> -#include <linux/kasan.h>
>
> /*
> * Note: test functions are marked noinline so that their names appear in
> @@ -623,6 +624,73 @@ static noinline void __init kasan_strings(void)
> strnlen(ptr, 1);
> }
>
> +static noinline void __init kasan_bitops(void)
> +{
> + long *bits = kzalloc(sizeof(*bits), GFP_KERNEL);
It would be safer to do kzalloc(sizeof(*bits) + 1, GFP_KERNEL) and change tests accordingly to: set_bit(BITS_PER_LONG + 1, bits) ...
kmalloc will internally round up allocation to 16-bytes, so we won't be actually corrupting someone elses memory.
> + if (!bits)
> + return;
> +
> + pr_info("within-bounds in set_bit");
> + set_bit(0, bits);
> +
> + pr_info("within-bounds in set_bit");
> + set_bit(BITS_PER_LONG - 1, bits);
I'd remove these two. There are plenty of within bounds set_bit() in the kernel so they are well tested already.
^ permalink raw reply
* Re: [RFC 4/7] KEYS: trusted: Introduce TEE based Trusted Keys
From: Jarkko Sakkinen @ 2019-06-13 15:32 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, jens.wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, ard.biesheuvel,
daniel.thompson, linux-doc, linux-kernel, tee-dev
In-Reply-To: <1560421833-27414-5-git-send-email-sumit.garg@linaro.org>
On Thu, Jun 13, 2019 at 04:00:30PM +0530, Sumit Garg wrote:
> Add support for TEE based trusted keys where TEE provides the functionality
> to seal and unseal trusted keys using hardware unique key.
>
> Refer to Documentation/tee.txt for detailed information about TEE.
>
> Approach taken in this patch acts as an alternative to a TPM device in case
> platform doesn't possess one.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
How does this interact with the trusted module? Why there is no update
to security/keys/trusted-encrypted.txt?
Somehow the existing trusted module needs to be re-architected to work
with either. Otherwise, this will turn out to be a mess.
/Jarkko
^ permalink raw reply
* Re: [PATCH v3 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Andrey Ryabinin @ 2019-06-13 10:51 UTC (permalink / raw)
To: Marco Elver, peterz, dvyukov, glider, andreyknvl, mark.rutland,
hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev
In-Reply-To: <20190531150828.157832-4-elver@google.com>
On 5/31/19 6:08 PM, Marco Elver wrote:
> This adds a new header to asm-generic to allow optionally instrumenting
> architecture-specific asm implementations of bitops.
>
> This change includes the required change for x86 as reference and
> changes the kernel API doc to point to bitops-instrumented.h instead.
> Rationale: the functions in x86's bitops.h are no longer the kernel API
> functions, but instead the arch_ prefixed functions, which are then
> instrumented via bitops-instrumented.h.
>
> Other architectures can similarly add support for asm implementations of
> bitops.
>
> The documentation text was derived from x86 and existing bitops
> asm-generic versions: 1) references to x86 have been removed; 2) as a
> result, some of the text had to be reworded for clarity and consistency.
>
> Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
> Signed-off-by: Marco Elver <elver@google.com>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
^ permalink raw reply
* Re: [PATCH v3 2/3] x86: Use static_cpu_has in uaccess region to avoid instrumentation
From: Andrey Ryabinin @ 2019-06-13 10:50 UTC (permalink / raw)
To: Marco Elver, peterz, dvyukov, glider, andreyknvl, mark.rutland,
hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev
In-Reply-To: <20190531150828.157832-3-elver@google.com>
On 5/31/19 6:08 PM, Marco Elver wrote:
> This patch is a pre-requisite for enabling KASAN bitops instrumentation;
> using static_cpu_has instead of boot_cpu_has avoids instrumentation of
> test_bit inside the uaccess region. With instrumentation, the KASAN
> check would otherwise be flagged by objtool.
>
> For consistency, kernel/signal.c was changed to mirror this change,
> however, is never instrumented with KASAN (currently unsupported under
> x86 32bit).
>
> Signed-off-by: Marco Elver <elver@google.com>
> Suggested-by: H. Peter Anvin <hpa@zytor.com>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Vincenzo Frascino @ 2019-06-13 11:16 UTC (permalink / raw)
To: Szabolcs Nagy, Catalin Marinas
Cc: nd, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, linux-mm@kvack.org,
linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, Will Deacon, Andrey Konovalov,
Alexander Viro
In-Reply-To: <dee7f192-d0f0-558e-3007-eba805c6f2da@arm.com>
Hi Szabolcs,
thank you for your review.
On 13/06/2019 11:14, Szabolcs Nagy wrote:
> On 13/06/2019 10:20, Catalin Marinas wrote:
>> Hi Szabolcs,
>>
>> On Wed, Jun 12, 2019 at 05:30:34PM +0100, Szabolcs Nagy wrote:
>>> On 12/06/2019 15:21, Vincenzo Frascino wrote:
>>>> +2. ARM64 Tagged Address ABI
>>>> +---------------------------
>>>> +
>>>> +From the kernel syscall interface prospective, we define, for the purposes
>>> ^^^^^^^^^^^
>>> perspective
>>>
>>>> +of this document, a "valid tagged pointer" as a pointer that either it has
>>>> +a zero value set in the top byte or it has a non-zero value, it is in memory
>>>> +ranges privately owned by a userspace process and it is obtained in one of
>>>> +the following ways:
>>>> + - mmap() done by the process itself, where either:
>>>> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
>>>> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
>>>> + file or "/dev/zero"
>>>
>>> this does not make it clear if MAP_FIXED or other flags are valid
>>> (there are many map flags i don't know, but at least fixed should work
>>> and stack/growsdown. i'd expect anything that's not incompatible with
>>> private|anon to work).
>>
>> Just to clarify, this document tries to define the memory ranges from
>> where tagged addresses can be passed into the kernel in the context
>> of TBI only (not MTE); that is for hwasan support. FIXED or GROWSDOWN
>> should not affect this.
>
> yes, so either the text should list MAP_* flags that don't affect
> the pointer tagging semantics or specify private|anon mapping
> with different wording.
>
Good point. Could you please propose a wording that would be suitable for this case?
>>>> + - a mapping below sbrk(0) done by the process itself
>>>
>>> doesn't the mmap rule cover this?
>>
>> IIUC it doesn't cover it as that's memory mapped by the kernel
>> automatically on access vs a pointer returned by mmap(). The statement
>> above talks about how the address is obtained by the user.
>
> ok i read 'mapping below sbrk' as an mmap (possibly MAP_FIXED)
> that happens to be below the heap area.
>
> i think "below sbrk(0)" is not the best term to use: there
> may be address range below the heap area that can be mmapped
> and thus below sbrk(0) and sbrk is a posix api not a linux
> syscall, the libc can implement it with mmap or whatever.
>
> i'm not sure what the right term for 'heap area' is
> (the address range between syscall(__NR_brk,0) at
> program startup and its current value?)
>
I used sbrk(0) with the meaning of "end of the process's data segment" not
implying that this is a syscall, but just as a useful way to identify the mapping.
I agree that it is a posix function implemented by libc but when it is used with
0 finds the current location of the program break, which can be changed by brk()
and depending on the new address passed to this syscall can have the effect of
allocating or deallocating memory.
Will changing sbrk(0) with "end of the process's data segment" make it more clear?
I will add what you are suggesting about the heap area.
>>>> + - any memory mapped by the kernel in the process's address space during
>>>> + creation and following the restrictions presented above (i.e. data, bss,
>>>> + stack).
>>>
>>> OK.
>>>
>>> Can a null pointer have a tag?
>>> (in case NULL is valid to pass to a syscall)
>>
>> Good point. I don't think it can. We may change this for MTE where we
>> give a hint tag but no hint address, however, this document only covers
>> TBI for now.
>
> OK.
>
>>>> +The ARM64 Tagged Address ABI is an opt-in feature, and an application can
>>>> +control it using the following prctl()s:
>>>> + - PR_SET_TAGGED_ADDR_CTRL: can be used to enable the Tagged Address ABI.
>>>> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
>>>> + Address ABI.
>>>> +
>>>> +As a consequence of invoking PR_SET_TAGGED_ADDR_CTRL prctl() by an applications,
>>>> +the ABI guarantees the following behaviours:
>>>> +
>>>> + - Every current or newly introduced syscall can accept any valid tagged
>>>> + pointers.
>>>> +
>>>> + - If a non valid tagged pointer is passed to a syscall then the behaviour
>>>> + is undefined.
>>>> +
>>>> + - Every valid tagged pointer is expected to work as an untagged one.
>>>> +
>>>> + - The kernel preserves any valid tagged pointers and returns them to the
>>>> + userspace unchanged in all the cases except the ones documented in the
>>>> + "Preserving tags" paragraph of tagged-pointers.txt.
>>>
>>> OK.
>>>
>>> i guess pointers of another process are not "valid tagged pointers"
>>> for the current one, so e.g. in ptrace the ptracer has to clear the
>>> tags before PEEK etc.
>>
>> Another good point. Are there any pros/cons here or use-cases? When we
>> add MTE support, should we handle this differently?
>
> i'm not sure what gdb does currently, but it has
> an 'address_significant' hook used at a few places
> that drops the tag on aarch64, so it probably
> avoids passing tagged pointer to ptrace.
>
> i was worried about strace which tries to print
> structs passed to syscalls and follow pointers in
> them which currently would work, but if we allow
> tags in syscalls then it needs some update.
> (i haven't checked the strace code though)
>>>>> +A definition of the meaning of tagged pointers on arm64 can be found in:
>>>> +Documentation/arm64/tagged-pointers.txt.
>>>> +
>>>> +3. ARM64 Tagged Address ABI Exceptions
>>>> +--------------------------------------
>>>> +
>>>> +The behaviours described in paragraph 2, with particular reference to the
>>>> +acceptance by the syscalls of any valid tagged pointer are not applicable
>>>> +to the following cases:
>>>> + - mmap() addr parameter.
>>>> + - mremap() new_address parameter.
>>>> + - prctl_set_mm() struct prctl_map fields.
>>>> + - prctl_set_mm_map() struct prctl_map fields.
>>>
>>> i don't understand the exception: does it mean that passing a tagged
>>> address to these syscalls is undefined?
>>
>> I'd say it's as undefined as it is right now without these patches. We
>> may be able to explain this better in the document.
>>
>
--
Regards,
Vincenzo
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Dave Martin @ 2019-06-13 11:37 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: Catalin Marinas, linux-arch, linux-doc, Szabolcs Nagy,
Andrey Konovalov, Will Deacon, linux-kernel, linux-mm,
Alexander Viro, linux-kselftest, linux-arm-kernel
In-Reply-To: <141c740a-94c2-2243-b6d1-b44ffee43791@arm.com>
On Thu, Jun 13, 2019 at 11:15:34AM +0100, Vincenzo Frascino wrote:
> Hi Catalin,
>
> On 12/06/2019 16:35, Catalin Marinas wrote:
> > Hi Vincenzo,
> >
> > Some minor comments below but it looks fine to me overall. Cc'ing
> > Szabolcs as well since I'd like a view from the libc people.
> >
>
> Thanks for this, I saw Szabolcs comments.
>
> > On Wed, Jun 12, 2019 at 03:21:10PM +0100, Vincenzo Frascino wrote:
> >> diff --git a/Documentation/arm64/tagged-address-abi.txt b/Documentation/arm64/tagged-address-abi.txt
> >> new file mode 100644
> >> index 000000000000..96e149e2c55c
> >> --- /dev/null
> >> +++ b/Documentation/arm64/tagged-address-abi.txt
[...]
> >> +Since it is not desirable to relax the ABI to allow tagged user addresses
> >> +into the kernel indiscriminately, arm64 provides a new sysctl interface
> >> +(/proc/sys/abi/tagged_addr) that is used to prevent the applications from
> >> +enabling the relaxed ABI and a new prctl() interface that can be used to
> >> +enable or disable the relaxed ABI.
> >> +
> >> +The sysctl is meant also for testing purposes in order to provide a simple
> >> +way for the userspace to verify the return error checking of the prctl()
> >> +command without having to reconfigure the kernel.
> >> +
> >> +The ABI properties are inherited by threads of the same application and
> >> +fork()'ed children but cleared when a new process is spawn (execve()).
> >
> > "spawned".
I'd just say "cleared by execve()."
"Spawn" suggests (v)fork+exec to me (at least, what's what "spawn" means on
certain other OSes).
> >
> > I guess you could drop these three paragraphs here and mention the
> > inheritance properties when introducing the prctl() below. You can also
> > mention the global sysctl switch after the prctl() was introduced.
> >
>
> I will move the last two (rewording them) to the _section_ 2, but I would still
> prefer the Introduction to give an overview of the solution as well.
>
> >> +
> >> +2. ARM64 Tagged Address ABI
> >> +---------------------------
> >> +
> >> +From the kernel syscall interface prospective, we define, for the purposes
> >> +of this document, a "valid tagged pointer" as a pointer that either it has
> >
> > "either has" (no 'it') sounds slightly better but I'm not a native
> > English speaker either.
> >
> >> +a zero value set in the top byte or it has a non-zero value, it is in memory
> >> +ranges privately owned by a userspace process and it is obtained in one of
> >> +the following ways:
> >> + - mmap() done by the process itself, where either:
> >> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
> >> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
> >> + file or "/dev/zero"
> >> + - a mapping below sbrk(0) done by the process itself
> >> + - any memory mapped by the kernel in the process's address space during
> >> + creation and following the restrictions presented above (i.e. data, bss,
> >> + stack).
> >> +
> >> +The ARM64 Tagged Address ABI is an opt-in feature, and an application can
> >> +control it using the following prctl()s:
> >> + - PR_SET_TAGGED_ADDR_CTRL: can be used to enable the Tagged Address ABI.
> >
> > enable or disable (not sure we need the latter but it doesn't heart).
> >
> > I'd add the arg2 description here as well.
> >
>
> Good point I missed this.
>
> >> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
> >> + Address ABI.
For both prctls, you should also document the zeroed arguments up to
arg5 (unless we get rid of the enforcement and just ignore them).
Is there a canonical way to detect whether this whole API/ABI is
available? (i.e., try to call this prctl / check for an HWCAP bit,
etc.)
[...]
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Catalin Marinas @ 2019-06-13 12:28 UTC (permalink / raw)
To: Dave Martin
Cc: Vincenzo Frascino, linux-arch, linux-doc, Szabolcs Nagy,
Andrey Konovalov, Will Deacon, linux-kernel, linux-mm,
Alexander Viro, linux-kselftest, linux-arm-kernel
In-Reply-To: <20190613113731.GY28398@e103592.cambridge.arm.com>
On Thu, Jun 13, 2019 at 12:37:32PM +0100, Dave P Martin wrote:
> On Thu, Jun 13, 2019 at 11:15:34AM +0100, Vincenzo Frascino wrote:
> > On 12/06/2019 16:35, Catalin Marinas wrote:
> > > On Wed, Jun 12, 2019 at 03:21:10PM +0100, Vincenzo Frascino wrote:
> > >> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
> > >> + Address ABI.
[...]
> Is there a canonical way to detect whether this whole API/ABI is
> available? (i.e., try to call this prctl / check for an HWCAP bit,
> etc.)
The canonical way is a prctl() call. HWCAP doesn't make sense since it's
not a hardware feature. If you really want a different way of detecting
this (which I don't think it's worth), we can reinstate the AT_FLAGS
bit.
--
Catalin
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Szabolcs Nagy @ 2019-06-13 12:28 UTC (permalink / raw)
To: Vincenzo Frascino, Catalin Marinas
Cc: nd, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, linux-mm@kvack.org,
linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, Will Deacon, Andrey Konovalov,
Alexander Viro
In-Reply-To: <6ebbda37-5dd9-d0d5-d9cb-286c7a5b7f8e@arm.com>
On 13/06/2019 12:16, Vincenzo Frascino wrote:
> Hi Szabolcs,
>
> thank you for your review.
>
> On 13/06/2019 11:14, Szabolcs Nagy wrote:
>> On 13/06/2019 10:20, Catalin Marinas wrote:
>>> Hi Szabolcs,
>>>
>>> On Wed, Jun 12, 2019 at 05:30:34PM +0100, Szabolcs Nagy wrote:
>>>> On 12/06/2019 15:21, Vincenzo Frascino wrote:
>>>>> +2. ARM64 Tagged Address ABI
>>>>> +---------------------------
>>>>> +
>>>>> +From the kernel syscall interface prospective, we define, for the purposes
>>>> ^^^^^^^^^^^
>>>> perspective
>>>>
>>>>> +of this document, a "valid tagged pointer" as a pointer that either it has
>>>>> +a zero value set in the top byte or it has a non-zero value, it is in memory
>>>>> +ranges privately owned by a userspace process and it is obtained in one of
>>>>> +the following ways:
>>>>> + - mmap() done by the process itself, where either:
>>>>> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
>>>>> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
>>>>> + file or "/dev/zero"
>>>>
>>>> this does not make it clear if MAP_FIXED or other flags are valid
>>>> (there are many map flags i don't know, but at least fixed should work
>>>> and stack/growsdown. i'd expect anything that's not incompatible with
>>>> private|anon to work).
>>>
>>> Just to clarify, this document tries to define the memory ranges from
>>> where tagged addresses can be passed into the kernel in the context
>>> of TBI only (not MTE); that is for hwasan support. FIXED or GROWSDOWN
>>> should not affect this.
>>
>> yes, so either the text should list MAP_* flags that don't affect
>> the pointer tagging semantics or specify private|anon mapping
>> with different wording.
>>
>
> Good point. Could you please propose a wording that would be suitable for this case?
i don't know all the MAP_ magic, but i think it's enough to change
the "flags =" to
* flags have MAP_PRIVATE and MAP_ANONYMOUS set or
* flags have MAP_PRIVATE set and the file descriptor refers to...
>>>>> + - a mapping below sbrk(0) done by the process itself
>>>>
>>>> doesn't the mmap rule cover this?
>>>
>>> IIUC it doesn't cover it as that's memory mapped by the kernel
>>> automatically on access vs a pointer returned by mmap(). The statement
>>> above talks about how the address is obtained by the user.
>>
>> ok i read 'mapping below sbrk' as an mmap (possibly MAP_FIXED)
>> that happens to be below the heap area.
>>
>> i think "below sbrk(0)" is not the best term to use: there
>> may be address range below the heap area that can be mmapped
>> and thus below sbrk(0) and sbrk is a posix api not a linux
>> syscall, the libc can implement it with mmap or whatever.
>>
>> i'm not sure what the right term for 'heap area' is
>> (the address range between syscall(__NR_brk,0) at
>> program startup and its current value?)
>>
>
> I used sbrk(0) with the meaning of "end of the process's data segment" not
> implying that this is a syscall, but just as a useful way to identify the mapping.
> I agree that it is a posix function implemented by libc but when it is used with
> 0 finds the current location of the program break, which can be changed by brk()
> and depending on the new address passed to this syscall can have the effect of
> allocating or deallocating memory.
>
> Will changing sbrk(0) with "end of the process's data segment" make it more clear?
i don't understand what's the relevance of the *end*
of the data segment.
i'd expect the text to say something about the address
range of the data segment.
i can do
mmap((void*)65536, 65536, PROT_READ|PROT_WRITE, MAP_FIXED|MAP_SHARED|MAP_ANON, -1, 0);
and it will be below the end of the data segment.
>
> I will add what you are suggesting about the heap area.
>
^ permalink raw reply
* [PATCH v4 0/3] Bitops instrumentation for KASAN
From: Marco Elver @ 2019-06-13 12:30 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
Previous version:
http://lkml.kernel.org/r/20190531150828.157832-1-elver@google.com
* This version only changes lib/test_kasan.c.
* Remaining files are identical to v3.
Marco Elver (3):
lib/test_kasan: Add bitops tests
x86: Use static_cpu_has in uaccess region to avoid instrumentation
asm-generic, x86: Add bitops instrumentation for KASAN
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/include/asm/bitops.h | 189 ++++------------
arch/x86/kernel/signal.c | 2 +-
include/asm-generic/bitops-instrumented.h | 263 ++++++++++++++++++++++
lib/test_kasan.c | 82 ++++++-
6 files changed, 383 insertions(+), 157 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox