[PATCH] Documentation: tpm_tis
Based recent discussions on LKML, provide preliminary bits of tpm_tis_core dependent drivers. Includes only bare essentials but can be extended later on case by case. This way some people may even want to read it later on. Cc: Jonathan Corbet CC: Daniel P. Smith Cc: Lino Sanfilippo Cc: Jason Gunthorpe Cc: Peter Huewe Cc: James Bottomley Cc: Alexander Steffen Cc: keyri...@vger.kernel.org Cc: linux-doc@vger.kernel.org Cc: linux-ker...@vger.kernel.org Cc: linux-integr...@vger.kernel.org Signed-off-by: Jarkko Sakkinen --- Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_tis.rst | 30 ++ 2 files changed, 31 insertions(+) create mode 100644 Documentation/security/tpm/tpm_tis.rst diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index fc40e9f23c85..f27a17f60a96 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -5,6 +5,7 @@ Trusted Platform Module documentation .. toctree:: tpm_event_log + tpm_tis tpm_vtpm_proxy xen-tpmfront tpm_ftpm_tee diff --git a/Documentation/security/tpm/tpm_tis.rst b/Documentation/security/tpm/tpm_tis.rst new file mode 100644 index ..3cec0216a169 --- /dev/null +++ b/Documentation/security/tpm/tpm_tis.rst @@ -0,0 +1,30 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM FIFO interface Driver += + +FIFO (First-In-First-Out) is the name of the hardware interface used by the +`tpm_tis_core` dependent drivers. The prefix "tis" is named after TPM +Interface Specification, which is the hardware interface specification for +TPM 1.x chips. + +Communication is based on a 5 KiB buffer shared by the TPM chip through a +hardware bus or memory map. The buffer is further split to five equal size +buffers, which provide equivalent sets of registers for communication +between CPU and TPM. The communication end points are called *localities* +in the TCG terminology. + +When a kernel wants to send a commands to the TPM chip, it first reserves +locality 0 by setting `requestUse` bit in `TPM_ACCESS` register. The bit is +cleared by the chip when the access is granted. Once completed its +communication, it sets `activeLocity` bit in the same register. + +Pending localities are served in order by the chip descending orderm and +one at a time: + +- Locality 0 has the lowest priority. +- Locality 5 has the highest priotiy. + +Further information on purpose and meaning of the localities can be found +from section 3.2 of TCG PC Client Platform TPM Profile Specification. -- 2.40.1
Re: [PATCH] Documentation: tpm_tis
Some remarks below that I noticed after sending this. On Mon Mar 4, 2024 at 11:27 PM EET, Jarkko Sakkinen wrote: > Based recent discussions on LKML, provide preliminary bits of tpm_tis_core s/Based/ Based on/ > dependent drivers. Includes only bare essentials but can be extended later > on case by case. This way some people may even want to read it later on. > > Cc: Jonathan Corbet > CC: Daniel P. Smith > Cc: Lino Sanfilippo > Cc: Jason Gunthorpe > Cc: Peter Huewe > Cc: James Bottomley > Cc: Alexander Steffen > Cc: keyri...@vger.kernel.org > Cc: linux-doc@vger.kernel.org > Cc: linux-ker...@vger.kernel.org > Cc: linux-integr...@vger.kernel.org > Signed-off-by: Jarkko Sakkinen > --- > Documentation/security/tpm/index.rst | 1 + > Documentation/security/tpm/tpm_tis.rst | 30 ++ > 2 files changed, 31 insertions(+) > create mode 100644 Documentation/security/tpm/tpm_tis.rst > > diff --git a/Documentation/security/tpm/index.rst > b/Documentation/security/tpm/index.rst > index fc40e9f23c85..f27a17f60a96 100644 > --- a/Documentation/security/tpm/index.rst > +++ b/Documentation/security/tpm/index.rst > @@ -5,6 +5,7 @@ Trusted Platform Module documentation > .. toctree:: > > tpm_event_log > + tpm_tis > tpm_vtpm_proxy > xen-tpmfront > tpm_ftpm_tee > diff --git a/Documentation/security/tpm/tpm_tis.rst > b/Documentation/security/tpm/tpm_tis.rst > new file mode 100644 > index ..3cec0216a169 > --- /dev/null > +++ b/Documentation/security/tpm/tpm_tis.rst > @@ -0,0 +1,30 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > += > +TPM FIFO interface Driver > += > + > +FIFO (First-In-First-Out) is the name of the hardware interface used by the > +`tpm_tis_core` dependent drivers. The prefix "tis" is named after TPM > +Interface Specification, which is the hardware interface specification for > +TPM 1.x chips. > + > +Communication is based on a 5 KiB buffer shared by the TPM chip through a > +hardware bus or memory map. The buffer is further split to five equal size s/to/into/ > +buffers, which provide equivalent sets of registers for communication > +between CPU and TPM. The communication end points are called *localities* > +in the TCG terminology. > + > +When a kernel wants to send a commands to the TPM chip, it first reserves s/a kernel/kernel/ s/a commands/commands/ > +locality 0 by setting `requestUse` bit in `TPM_ACCESS` register. The bit is > +cleared by the chip when the access is granted. Once completed its > +communication, it sets `activeLocity` bit in the same register. s/it sets/kernel relinquishes reservation by setting/ > + > +Pending localities are served in order by the chip descending orderm and > +one at a time: "Pending localities are served in descending order and one at a time:" > + > +- Locality 0 has the lowest priority. > +- Locality 5 has the highest priotiy. > + > +Further information on purpose and meaning of the localities can be found > +from section 3.2 of TCG PC Client Platform TPM Profile Specification. s/on purpose/on the purpose/ BR, Jarkko
Re: [PATCH] Documentation: tpm_tis
On Tue Mar 5, 2024 at 12:53 AM EET, Randy Dunlap wrote: > > > On 3/4/24 13:27, Jarkko Sakkinen wrote: > > Based recent discussions on LKML, provide preliminary bits of tpm_tis_core > > dependent drivers. Includes only bare essentials but can be extended later > > on case by case. This way some people may even want to read it later on. > > > > Cc: Jonathan Corbet > > CC: Daniel P. Smith > > Cc: Lino Sanfilippo > > Cc: Jason Gunthorpe > > Cc: Peter Huewe > > Cc: James Bottomley > > Cc: Alexander Steffen > > Cc: keyri...@vger.kernel.org > > Cc: linux-doc@vger.kernel.org > > Cc: linux-ker...@vger.kernel.org > > Cc: linux-integr...@vger.kernel.org > > Signed-off-by: Jarkko Sakkinen > > --- > > Documentation/security/tpm/index.rst | 1 + > > Documentation/security/tpm/tpm_tis.rst | 30 ++ > > 2 files changed, 31 insertions(+) > > create mode 100644 Documentation/security/tpm/tpm_tis.rst > > > > > diff --git a/Documentation/security/tpm/tpm_tis.rst > > b/Documentation/security/tpm/tpm_tis.rst > > new file mode 100644 > > index ..3cec0216a169 > > --- /dev/null > > +++ b/Documentation/security/tpm/tpm_tis.rst > > @@ -0,0 +1,30 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > += > > +TPM FIFO interface Driver > > += > > + > > +FIFO (First-In-First-Out) is the name of the hardware interface used by the > > +`tpm_tis_core` dependent drivers. The prefix "tis" is named after TPM > > +Interface Specification, which is the hardware interface specification for > > +TPM 1.x chips. > > + > > +Communication is based on a 5 KiB buffer shared by the TPM chip through a > > +hardware bus or memory map. The buffer is further split to five equal size > > +buffers, which provide equivalent sets of registers for communication > > +between CPU and TPM. The communication end points are called *localities* > > +in the TCG terminology. > > + > > +When a kernel wants to send a commands to the TPM chip, it first reserves > > +locality 0 by setting `requestUse` bit in `TPM_ACCESS` register. The bit is > > +cleared by the chip when the access is granted. Once completed its > > +communication, it sets `activeLocity` bit in the same register. > > Is that activeLocality ? Yes. > > > + > > +Pending localities are served in order by the chip descending orderm and > > +one at a time: > > + > > +- Locality 0 has the lowest priority. > > +- Locality 5 has the highest priotiy. > > priority. > > > + > > +Further information on purpose and meaning of the localities can be found > > +from section 3.2 of TCG PC Client Platform TPM Profile Specification. Thanks for the remarks. Too many typos but at least I think the story is is understandable and describes pretty well key elements of tpm_tis_core. BR, Jarkko
[PATCH v2] Documentation: tpm_tis
Based recent discussions on LKML, provide preliminary bits of tpm_tis_core dependent drivers. Includes only bare essentials but can be extended later on case by case. This way some people may even want to read it later on. Cc: Jonathan Corbet CC: Daniel P. Smith Cc: Lino Sanfilippo Cc: Jason Gunthorpe Cc: Peter Huewe Cc: James Bottomley Cc: Alexander Steffen Cc: keyri...@vger.kernel.org Cc: linux-doc@vger.kernel.org Cc: linux-ker...@vger.kernel.org Cc: linux-integr...@vger.kernel.org Cc: Randy Dunlap Signed-off-by: Jarkko Sakkinen --- v2: - Fixed errors reported by Randy: https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ - Improved the text a bit to have a better presentation. --- Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_tis.rst | 30 ++ 2 files changed, 31 insertions(+) create mode 100644 Documentation/security/tpm/tpm_tis.rst diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index fc40e9f23c85..f27a17f60a96 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -5,6 +5,7 @@ Trusted Platform Module documentation .. toctree:: tpm_event_log + tpm_tis tpm_vtpm_proxy xen-tpmfront tpm_ftpm_tee diff --git a/Documentation/security/tpm/tpm_tis.rst b/Documentation/security/tpm/tpm_tis.rst new file mode 100644 index ..b331813b3c45 --- /dev/null +++ b/Documentation/security/tpm/tpm_tis.rst @@ -0,0 +1,30 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM FIFO interface Driver += + +FIFO (First-In-First-Out) is the name of the hardware interface used by the +tpm_tis_core dependent drivers. The prefix "tis" comes from the TPM Interface +Specification, which is the hardware interface specification for TPM 1.x chips. + +Communication is based on a 5 KiB buffer shared by the TPM chip through a +hardware bus or memory map, depending on the physical wiring. The buffer is +further split into five equal-size buffers, which provide equivalent sets of +registers for communication between the CPU and TPM. These communication +endpoints are called localities in the TCG terminology. + +When the kernel wants to send commands to the TPM chip, it first reserves +locality 0 by setting the requestUse bit in the TPM_ACCESS register. The bit is +cleared by the chip when the access is granted. Once it completes its +communication, the kernel writes the TPM_ACCESS.activeLocality bit. This +informs the chip that the locality has been relinquished. + +Pending localities are served in order by the chip in descending order, one at +a time: + +- Locality 0 has the lowest priority. +- Locality 5 has the highest priority. + +Further information on the purpose and meaning of the localities can be found +in section 3.2 of the TCG PC Client Platform TPM Profile Specification. -- 2.44.0
Re: [PATCH v2] Documentation: tpm_tis
On Wed Mar 20, 2024 at 6:15 PM EET, Stefan Berger wrote: > > > On 3/20/24 04:56, Jarkko Sakkinen wrote: > > Based recent discussions on LKML, provide preliminary bits of tpm_tis_core > > dependent drivers. Includes only bare essentials but can be extended later > > on case by case. This way some people may even want to read it later on. > > > > Cc: Jonathan Corbet > > CC: Daniel P. Smith > > Cc: Lino Sanfilippo > > Cc: Jason Gunthorpe > > Cc: Peter Huewe > > Cc: James Bottomley > > Cc: Alexander Steffen > > Cc: keyri...@vger.kernel.org > > Cc: linux-doc@vger.kernel.org > > Cc: linux-ker...@vger.kernel.org > > Cc: linux-integr...@vger.kernel.org > > Cc: Randy Dunlap > > Signed-off-by: Jarkko Sakkinen > > --- > > v2: > > - Fixed errors reported by Randy: > > > > https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ > > - Improved the text a bit to have a better presentation. > > --- > > Documentation/security/tpm/index.rst | 1 + > > Documentation/security/tpm/tpm_tis.rst | 30 ++ > > 2 files changed, 31 insertions(+) > > create mode 100644 Documentation/security/tpm/tpm_tis.rst > > > > diff --git a/Documentation/security/tpm/index.rst > > b/Documentation/security/tpm/index.rst > > index fc40e9f23c85..f27a17f60a96 100644 > > --- a/Documentation/security/tpm/index.rst > > +++ b/Documentation/security/tpm/index.rst > > @@ -5,6 +5,7 @@ Trusted Platform Module documentation > > .. toctree:: > > > > tpm_event_log > > + tpm_tis > > tpm_vtpm_proxy > > xen-tpmfront > > tpm_ftpm_tee > > diff --git a/Documentation/security/tpm/tpm_tis.rst > > b/Documentation/security/tpm/tpm_tis.rst > > new file mode 100644 > > index ..b331813b3c45 > > --- /dev/null > > +++ b/Documentation/security/tpm/tpm_tis.rst > > @@ -0,0 +1,30 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > += > > +TPM FIFO interface Driver > > += > > + > > +FIFO (First-In-First-Out) is the name of the hardware interface used by the > > FIFO is the type. I am surprised you call it a 'name'. I would say TIS > is the 'name'. It's what the official specification calls it [1]. > > > +tpm_tis_core dependent drivers. The prefix "tis" comes from the TPM > > Interface > > tis is a tla -- a three letter *acronym*. You aren't using it as a 'prefix'. I don't know what "tla" means. > > > +Specification, which is the hardware interface specification for TPM 1.x > > chips. > > It's also available for TPM2. Yes, but TIS is the name used by the legacy specification. > > > + > > +Communication is based on a 5 KiB buffer shared by the TPM chip through a > > I thought it was typically 4 KiB. You are basing this on table 9 in [1]? > > > +hardware bus or memory map, depending on the physical wiring. The buffer is > > +further split into five equal-size buffers, which provide equivalent sets > > of > > equal-sized MMIO regions? I'm not sure what spec you are referring to but [1] defines also other communication paths. > > > +registers for communication between the CPU and TPM. These communication > > +endpoints are called localities in the TCG terminology. > > + > > +When the kernel wants to send commands to the TPM chip, it first reserves > > +locality 0 by setting the requestUse bit in the TPM_ACCESS register. The > > bit is > > +cleared by the chip when the access is granted. Once it completes its > > +communication, the kernel writes the TPM_ACCESS.activeLocality bit. This > > +informs the chip that the locality has been relinquished. > > + > > +Pending localities are served in order by the chip in descending order, > > one at > > +a time: > > I think I know what pending localities are because I have worked with > this device but I am not sure whether the user can deduce this from the > paragraph above. Also, why this particular detail when the driver only > uses locality 0 and nobody is competing about access to localities? This is pretty good summary that is IMHO somewhat useful. You are welcome to contribute to the documentation but it has to start from something. > > > + > > +- Locality 0 has the lowest priority. > > +- Locality 5 has the highest priority. > > + > > +Further information on the purpose and meaning of the localities can be > > found > > +in section 3.2 of the TCG PC Client Platform TPM Profile Specification. o [1] https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/ BR, Jarkko
Re: [PATCH v2] Documentation: tpm_tis
On Wed Mar 20, 2024 at 4:27 PM EET, Randy Dunlap wrote: > Hi, > > On 3/20/24 01:56, Jarkko Sakkinen wrote: > > Based recent discussions on LKML, provide preliminary bits of tpm_tis_core > > dependent drivers. Includes only bare essentials but can be extended later > > on case by case. This way some people may even want to read it later on. > > > > Cc: Jonathan Corbet > > CC: Daniel P. Smith > > Cc: Lino Sanfilippo > > Cc: Jason Gunthorpe > > Cc: Peter Huewe > > Cc: James Bottomley > > Cc: Alexander Steffen > > Cc: keyri...@vger.kernel.org > > Cc: linux-doc@vger.kernel.org > > Cc: linux-ker...@vger.kernel.org > > Cc: linux-integr...@vger.kernel.org > > Cc: Randy Dunlap > > Signed-off-by: Jarkko Sakkinen > > Makes sense to me. Thanks. > > Reviewed-by: Randy Dunlap Good to hear, I tried to write a summary that makes common sense :-) Thanks for the review! BR, Jarkko
Re: [PATCH v2] Documentation: tpm_tis
On Thu Mar 21, 2024 at 6:09 PM EET, Stefan Berger wrote: > > > On 3/21/24 11:51, Jarkko Sakkinen wrote: > > On Wed Mar 20, 2024 at 6:15 PM EET, Stefan Berger wrote: > >> > >> > >> On 3/20/24 04:56, Jarkko Sakkinen wrote: > >>> Based recent discussions on LKML, provide preliminary bits of tpm_tis_core > >>> dependent drivers. Includes only bare essentials but can be extended later > >>> on case by case. This way some people may even want to read it later on. > >>> > >>> Cc: Jonathan Corbet > >>> CC: Daniel P. Smith > >>> Cc: Lino Sanfilippo > >>> Cc: Jason Gunthorpe > >>> Cc: Peter Huewe > >>> Cc: James Bottomley > >>> Cc: Alexander Steffen > >>> Cc: keyri...@vger.kernel.org > >>> Cc: linux-doc@vger.kernel.org > >>> Cc: linux-ker...@vger.kernel.org > >>> Cc: linux-integr...@vger.kernel.org > >>> Cc: Randy Dunlap > >>> Signed-off-by: Jarkko Sakkinen > >>> --- > >>> v2: > >>> - Fixed errors reported by Randy: > >>> > >>> https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ > >>> - Improved the text a bit to have a better presentation. > >>> --- > >>>Documentation/security/tpm/index.rst | 1 + > >>>Documentation/security/tpm/tpm_tis.rst | 30 ++ > >>>2 files changed, 31 insertions(+) > >>>create mode 100644 Documentation/security/tpm/tpm_tis.rst > >>> > >>> diff --git a/Documentation/security/tpm/index.rst > >>> b/Documentation/security/tpm/index.rst > >>> index fc40e9f23c85..f27a17f60a96 100644 > >>> --- a/Documentation/security/tpm/index.rst > >>> +++ b/Documentation/security/tpm/index.rst > >>> @@ -5,6 +5,7 @@ Trusted Platform Module documentation > >>>.. toctree:: > >>> > >>> tpm_event_log > >>> + tpm_tis > >>> tpm_vtpm_proxy > >>> xen-tpmfront > >>> tpm_ftpm_tee > >>> diff --git a/Documentation/security/tpm/tpm_tis.rst > >>> b/Documentation/security/tpm/tpm_tis.rst > >>> new file mode 100644 > >>> index ..b331813b3c45 > >>> --- /dev/null > >>> +++ b/Documentation/security/tpm/tpm_tis.rst > >>> @@ -0,0 +1,30 @@ > >>> +.. SPDX-License-Identifier: GPL-2.0 > >>> + > >>> += > >>> +TPM FIFO interface Driver > >>> += > >>> + > >>> +FIFO (First-In-First-Out) is the name of the hardware interface used by > >>> the > >> > >> FIFO is the type. I am surprised you call it a 'name'. I would say TIS > >> is the 'name'. > > > > It's what the official specification calls it [1]. > > > > > >> > >>> +tpm_tis_core dependent drivers. The prefix "tis" comes from the TPM > >>> Interface > >> > >> tis is a tla -- a three letter *acronym*. You aren't using it as a > >> 'prefix'. > > > > I don't know what "tla" means. > > > >> > >>> +Specification, which is the hardware interface specification for TPM 1.x > >>> chips. > >> > >> It's also available for TPM2. > > > > Yes, but TIS is the name used by the legacy specification. > > > The point is that TIS is not just a TPM 1.x interface but also used for > TPM 2. FIFO interface is what is used in the spec so I'll stick to that. > > > >> > >>> + > >>> +Communication is based on a 5 KiB buffer shared by the TPM chip through a > >> > >> I thought it was typically 4 KiB. > > > > You are basing this on table 9 in [1]? > > Yes. See below. > > > > >> > >>> +hardware bus or memory map, depending on the physical wiring. The buffer > >>> is > >>> +further split into five equal-size buffers, which provide equivalent > >>> sets of > > If you are referring to the MMIO region between 0xfed4 and 0xfed4 > 4fff as a buffer then you are talking about a **20kb** MMIO region > (0x5000) that is **split** into equal-sized MMIO regions, each having > 4kb (0x1000). Yes, that's the 4kb then but there that one is no 5kb > 'further split into five equal-sized buffers' of presumably 1kb each. > Each locality has a 0x1000 sized MMIO region. Oops, true! I'll fix this part thanks, had a blind spot :-) Will fix for v3. BR, Jarkko
Re: [PATCH v2] Documentation: tpm_tis
On Thu Mar 21, 2024 at 6:24 PM EET, Jarkko Sakkinen wrote: > On Thu Mar 21, 2024 at 6:09 PM EET, Stefan Berger wrote: > > > > > > On 3/21/24 11:51, Jarkko Sakkinen wrote: > > > On Wed Mar 20, 2024 at 6:15 PM EET, Stefan Berger wrote: > > >> > > >> > > >> On 3/20/24 04:56, Jarkko Sakkinen wrote: > > >>> Based recent discussions on LKML, provide preliminary bits of > > >>> tpm_tis_core > > >>> dependent drivers. Includes only bare essentials but can be extended > > >>> later > > >>> on case by case. This way some people may even want to read it later on. > > >>> > > >>> Cc: Jonathan Corbet > > >>> CC: Daniel P. Smith > > >>> Cc: Lino Sanfilippo > > >>> Cc: Jason Gunthorpe > > >>> Cc: Peter Huewe > > >>> Cc: James Bottomley > > >>> Cc: Alexander Steffen > > >>> Cc: keyri...@vger.kernel.org > > >>> Cc: linux-doc@vger.kernel.org > > >>> Cc: linux-ker...@vger.kernel.org > > >>> Cc: linux-integr...@vger.kernel.org > > >>> Cc: Randy Dunlap > > >>> Signed-off-by: Jarkko Sakkinen > > >>> --- > > >>> v2: > > >>> - Fixed errors reported by Randy: > > >>> > > >>> https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ > > >>> - Improved the text a bit to have a better presentation. > > >>> --- > > >>>Documentation/security/tpm/index.rst | 1 + > > >>>Documentation/security/tpm/tpm_tis.rst | 30 > > >>> ++ > > >>>2 files changed, 31 insertions(+) > > >>>create mode 100644 Documentation/security/tpm/tpm_tis.rst > > >>> > > >>> diff --git a/Documentation/security/tpm/index.rst > > >>> b/Documentation/security/tpm/index.rst > > >>> index fc40e9f23c85..f27a17f60a96 100644 > > >>> --- a/Documentation/security/tpm/index.rst > > >>> +++ b/Documentation/security/tpm/index.rst > > >>> @@ -5,6 +5,7 @@ Trusted Platform Module documentation > > >>>.. toctree:: > > >>> > > >>> tpm_event_log > > >>> + tpm_tis > > >>> tpm_vtpm_proxy > > >>> xen-tpmfront > > >>> tpm_ftpm_tee > > >>> diff --git a/Documentation/security/tpm/tpm_tis.rst > > >>> b/Documentation/security/tpm/tpm_tis.rst > > >>> new file mode 100644 > > >>> index ..b331813b3c45 > > >>> --- /dev/null > > >>> +++ b/Documentation/security/tpm/tpm_tis.rst > > >>> @@ -0,0 +1,30 @@ > > >>> +.. SPDX-License-Identifier: GPL-2.0 > > >>> + > > >>> += > > >>> +TPM FIFO interface Driver > > >>> += > > >>> + > > >>> +FIFO (First-In-First-Out) is the name of the hardware interface used > > >>> by the > > >> > > >> FIFO is the type. I am surprised you call it a 'name'. I would say TIS > > >> is the 'name'. > > > > > > It's what the official specification calls it [1]. > > > > > > > > >> > > >>> +tpm_tis_core dependent drivers. The prefix "tis" comes from the TPM > > >>> Interface > > >> > > >> tis is a tla -- a three letter *acronym*. You aren't using it as a > > >> 'prefix'. > > > > > > I don't know what "tla" means. > > > > > >> > > >>> +Specification, which is the hardware interface specification for TPM > > >>> 1.x chips. > > >> > > >> It's also available for TPM2. > > > > > > Yes, but TIS is the name used by the legacy specification. > > > > > > The point is that TIS is not just a TPM 1.x interface but also used for > > TPM 2. > > > FIFO interface is what is used in the spec so I'll stick to that. E.g. Table 15 - *FIFO* Interface Identifier Register Not *TIS* Inteface Identifier Register. I don't want to invent my own terminology here and this the spec that we usually refer in every possible discussion around the topic. BR, Jarkko
Re: [PATCH v2] Documentation: tpm_tis
On Thu Mar 21, 2024 at 6:32 PM EET, Jarkko Sakkinen wrote: > On Thu Mar 21, 2024 at 6:24 PM EET, Jarkko Sakkinen wrote: > > On Thu Mar 21, 2024 at 6:09 PM EET, Stefan Berger wrote: > > > > > > > > > On 3/21/24 11:51, Jarkko Sakkinen wrote: > > > > On Wed Mar 20, 2024 at 6:15 PM EET, Stefan Berger wrote: > > > >> > > > >> > > > >> On 3/20/24 04:56, Jarkko Sakkinen wrote: > > > >>> Based recent discussions on LKML, provide preliminary bits of > > > >>> tpm_tis_core > > > >>> dependent drivers. Includes only bare essentials but can be extended > > > >>> later > > > >>> on case by case. This way some people may even want to read it later > > > >>> on. > > > >>> > > > >>> Cc: Jonathan Corbet > > > >>> CC: Daniel P. Smith > > > >>> Cc: Lino Sanfilippo > > > >>> Cc: Jason Gunthorpe > > > >>> Cc: Peter Huewe > > > >>> Cc: James Bottomley > > > >>> Cc: Alexander Steffen > > > >>> Cc: keyri...@vger.kernel.org > > > >>> Cc: linux-doc@vger.kernel.org > > > >>> Cc: linux-ker...@vger.kernel.org > > > >>> Cc: linux-integr...@vger.kernel.org > > > >>> Cc: Randy Dunlap > > > >>> Signed-off-by: Jarkko Sakkinen > > > >>> --- > > > >>> v2: > > > >>> - Fixed errors reported by Randy: > > > >>> > > > >>> https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ > > > >>> - Improved the text a bit to have a better presentation. > > > >>> --- > > > >>>Documentation/security/tpm/index.rst | 1 + > > > >>>Documentation/security/tpm/tpm_tis.rst | 30 > > > >>> ++ > > > >>>2 files changed, 31 insertions(+) > > > >>>create mode 100644 Documentation/security/tpm/tpm_tis.rst > > > >>> > > > >>> diff --git a/Documentation/security/tpm/index.rst > > > >>> b/Documentation/security/tpm/index.rst > > > >>> index fc40e9f23c85..f27a17f60a96 100644 > > > >>> --- a/Documentation/security/tpm/index.rst > > > >>> +++ b/Documentation/security/tpm/index.rst > > > >>> @@ -5,6 +5,7 @@ Trusted Platform Module documentation > > > >>>.. toctree:: > > > >>> > > > >>> tpm_event_log > > > >>> + tpm_tis > > > >>> tpm_vtpm_proxy > > > >>> xen-tpmfront > > > >>> tpm_ftpm_tee > > > >>> diff --git a/Documentation/security/tpm/tpm_tis.rst > > > >>> b/Documentation/security/tpm/tpm_tis.rst > > > >>> new file mode 100644 > > > >>> index ..b331813b3c45 > > > >>> --- /dev/null > > > >>> +++ b/Documentation/security/tpm/tpm_tis.rst > > > >>> @@ -0,0 +1,30 @@ > > > >>> +.. SPDX-License-Identifier: GPL-2.0 > > > >>> + > > > >>> += > > > >>> +TPM FIFO interface Driver > > > >>> += > > > >>> + > > > >>> +FIFO (First-In-First-Out) is the name of the hardware interface used > > > >>> by the > > > >> > > > >> FIFO is the type. I am surprised you call it a 'name'. I would say TIS > > > >> is the 'name'. > > > > > > > > It's what the official specification calls it [1]. > > > > > > > > > > > >> > > > >>> +tpm_tis_core dependent drivers. The prefix "tis" comes from the TPM > > > >>> Interface > > > >> > > > >> tis is a tla -- a three letter *acronym*. You aren't using it as a > > > >> 'prefix'. > > > > > > > > I don't know what "tla" means. > > > > > > > >> > > > >>> +Specification, which is the hardware interface specification for TPM > > > >>> 1.x chips. > > > >> > > > >> It's also available for TPM2. > > > > > > > > Yes, but TIS is the name used by the legacy specification. > > > > > > > > > The point is that TIS is not just a TPM 1.x interface but also used for > > > TPM 2. > > > > > > FIFO interface is what is used in the spec so I'll stick to that. > > E.g. Table 15 - *FIFO* Interface Identifier Register > > Not *TIS* Inteface Identifier Register. > > I don't want to invent my own terminology here and this the spec > that we usually refer in every possible discussion around the topic. That table actually also clarifies this pretty well, see interface type: – FIFO interface as defined in PTP for TPM 2.0 is active. 0001 – CRB interface is active. – FIFO interface as defined in TIS1.3 is active (all other fields of this register are don’t care). E.g. FIFO interface can be configured according to TIS 1.3 specification but the interface is still referred as FIFO interface in the current spec. BR, Jarkko
[PATCH v3] Documentation: tpm_tis
Based recent discussions on LKML, provide preliminary bits of tpm_tis_core dependent drivers. Includes only bare essentials but can be extended later on case by case. This way some people may even want to read it later on. Cc: Jonathan Corbet CC: Daniel P. Smith Cc: Lino Sanfilippo Cc: Jason Gunthorpe Cc: Peter Huewe Cc: James Bottomley Cc: Alexander Steffen Cc: keyri...@vger.kernel.org Cc: linux-doc@vger.kernel.org Cc: linux-ker...@vger.kernel.org Cc: linux-integr...@vger.kernel.org Reviewed-by: Randy Dunlap Signed-off-by: Jarkko Sakkinen --- v3: - Fixed incorrect buffer size: https://lore.kernel.org/linux-integrity/d957dbd3-4975-48d7-abc5-1a01c0959...@linux.ibm.com/ v2: - Fixed errors reported by Randy: https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ - Improved the text a bit to have a better presentation. --- Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_tis.rst | 30 ++ 2 files changed, 31 insertions(+) create mode 100644 Documentation/security/tpm/tpm_tis.rst diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index fc40e9f23c85..f27a17f60a96 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -5,6 +5,7 @@ Trusted Platform Module documentation .. toctree:: tpm_event_log + tpm_tis tpm_vtpm_proxy xen-tpmfront tpm_ftpm_tee diff --git a/Documentation/security/tpm/tpm_tis.rst b/Documentation/security/tpm/tpm_tis.rst new file mode 100644 index ..078b75666086 --- /dev/null +++ b/Documentation/security/tpm/tpm_tis.rst @@ -0,0 +1,30 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM FIFO interface Driver += + +FIFO (First-In-First-Out) is the name of the hardware interface used by the +tpm_tis_core dependent drivers. The prefix "tis" comes from the TPM Interface +Specification, which is the hardware interface specification for TPM 1.x chips. + +Communication is based on a 20 KiB buffer shared by the TPM chip through a +hardware bus or memory map, depending on the physical wiring. The buffer is +further split into five equal-size 4 KiB buffers, which provide equivalent +sets of registers for communication between the CPU and TPM. These +communication endpoints are called localities in the TCG terminology. + +When the kernel wants to send commands to the TPM chip, it first reserves +locality 0 by setting the requestUse bit in the TPM_ACCESS register. The bit is +cleared by the chip when the access is granted. Once it completes its +communication, the kernel writes the TPM_ACCESS.activeLocality bit. This +informs the chip that the locality has been relinquished. + +Pending localities are served in order by the chip in descending order, one at +a time: + +- Locality 0 has the lowest priority. +- Locality 5 has the highest priority. + +Further information on the purpose and meaning of the localities can be found +in section 3.2 of the TCG PC Client Platform TPM Profile Specification. -- 2.43.0
Re: [PATCH v3] Documentation: tpm_tis
On Thu Mar 21, 2024 at 6:43 PM EET, Jarkko Sakkinen wrote: > Based recent discussions on LKML, provide preliminary bits of tpm_tis_core > dependent drivers. Includes only bare essentials but can be extended later > on case by case. This way some people may even want to read it later on. $ pdftotext PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf $ grep -ci 'FIFO interface' PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.txt 55 $ grep -ci 'TIS interface' PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.txt 2 55 > 2 so that pretty much nails this terminology. BR, Jarkko
Re: [PATCH v3] Documentation: tpm_tis
On Thu Mar 21, 2024 at 6:54 PM EET, Jarkko Sakkinen wrote: > On Thu Mar 21, 2024 at 6:43 PM EET, Jarkko Sakkinen wrote: > > Based recent discussions on LKML, provide preliminary bits of tpm_tis_core > > dependent drivers. Includes only bare essentials but can be extended later > > on case by case. This way some people may even want to read it later on. > > $ pdftotext > PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf > $ grep -ci 'FIFO interface' > PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.txt > 55 > > $ grep -ci 'TIS interface' > PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.txt > 2 > > 55 > 2 so that pretty much nails this terminology. To add, this documentation *clears* the confusion in "FIFO vs TIS" by documenting where TIS comes from (i.e. from the original TPM Interface Spefication). If you read the current standards you bump quite often (55 times in the current spec) to FIFO interface, so it is good to clarify their relation in the kernel documentation. BR, Jarkko
[PATCH v4] Documentation: tpm_tis
Based recent discussions on LKML, provide preliminary bits of tpm_tis_core dependent drivers. Includes only bare essentials but can be extended later on case by case. This way some people may even want to read it later on. Cc: Jonathan Corbet CC: Daniel P. Smith Cc: Lino Sanfilippo Cc: Jason Gunthorpe Cc: Peter Huewe Cc: James Bottomley Cc: Alexander Steffen Cc: keyri...@vger.kernel.org Cc: linux-doc@vger.kernel.org Cc: linux-ker...@vger.kernel.org Cc: linux-integr...@vger.kernel.org Cc: Randy Dunlap Signed-off-by: Jarkko Sakkinen --- v4: - Extended the text to address some of Stefan's concerns with v2. - Had to unfortunately remove Randy's reviewed-by because of this, given the amount of text added. v3: - Fixed incorrect buffer size: https://lore.kernel.org/linux-integrity/d957dbd3-4975-48d7-abc5-1a01c0959...@linux.ibm.com/ v2: - Fixed errors reported by Randy: https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ - Improved the text a bit to have a better presentation. --- Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_tis.rst | 46 ++ 2 files changed, 47 insertions(+) create mode 100644 Documentation/security/tpm/tpm_tis.rst diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index fc40e9f23c85..f27a17f60a96 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -5,6 +5,7 @@ Trusted Platform Module documentation .. toctree:: tpm_event_log + tpm_tis tpm_vtpm_proxy xen-tpmfront tpm_ftpm_tee diff --git a/Documentation/security/tpm/tpm_tis.rst b/Documentation/security/tpm/tpm_tis.rst new file mode 100644 index ..b448ea3db71d --- /dev/null +++ b/Documentation/security/tpm/tpm_tis.rst @@ -0,0 +1,46 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM FIFO interface driver += + +TCG PTP Specification defines two interface types: FIFO and CRB. The former is +based on sequenced read and write operations, and the latter is based on a +buffer containing the full command or response. + +FIFO (First-In-First-Out) interface is used by the tpm_tis_core dependent +drivers. Originally Linux had only a driver called tpm_tis, which covered +memory mapped (aka MMIO) interface but it was later on extended to cover other +physical interfaces supported by the TCG standard. + +For legacy compliance the original MMIO driver is called tpm_tis and the +framework for FIFO drivers is named as tpm_tis_core. The postfix "tis" in +tpm_tis comes from the TPM Interface Specification, which is the hardware +interface specification for TPM 1.x chips. + +Communication is based on a 20 KiB buffer shared by the TPM chip through a +hardware bus or memory map, depending on the physical wiring. The buffer is +further split into five equal-size 4 KiB buffers, which provide equivalent +sets of registers for communication between the CPU and TPM. These +communication endpoints are called localities in the TCG terminology. + +When the kernel wants to send commands to the TPM chip, it first reserves +locality 0 by setting the requestUse bit in the TPM_ACCESS register. The bit is +cleared by the chip when the access is granted. Once it completes its +communication, the kernel writes the TPM_ACCESS.activeLocality bit. This +informs the chip that the locality has been relinquished. + +Pending localities are served in order by the chip in descending order, one at +a time: + +- Locality 0 has the lowest priority. +- Locality 5 has the highest priority. + +Further information on the purpose and meaning of the localities can be found +in section 3.2 of the TCG PC Client Platform TPM Profile Specification. + +References +== + +TCG PC Client Platform TPM Profile (PTP) Specification +https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/ -- 2.43.0
Re: [PATCH v4] Documentation: tpm_tis
On Sat Mar 23, 2024 at 12:52 AM EET, Jakub Kicinski wrote: > On Fri, 22 Mar 2024 14:35:36 +0200 Jarkko Sakkinen wrote: > > +TCG PTP Specification defines two interface types: FIFO and CRB. The > > former is > > Could be worth spelling out the PTP part here, I'm guessing > get_maintainer made you CC netdev because it thought it stands > for Precision Time Protocol. And one has to read till the end > to see: > > > +TCG PC Client Platform TPM Profile (PTP) Specification Thanks! Good remark. BR, Jarkko
Re: [PATCH v4] Documentation: tpm_tis
On Sat Mar 23, 2024 at 2:39 AM EET, Daniel P. Smith wrote: > Hi Jarkko, > > On 3/22/24 08:35, Jarkko Sakkinen wrote: > > Based recent discussions on LKML, provide preliminary bits of tpm_tis_core > > dependent drivers. Includes only bare essentials but can be extended later > > on case by case. This way some people may even want to read it later on. > > > > Cc: Jonathan Corbet > > CC: Daniel P. Smith > > Cc: Lino Sanfilippo > > Cc: Jason Gunthorpe > > Cc: Peter Huewe > > Cc: James Bottomley > > Cc: Alexander Steffen > > Cc: keyri...@vger.kernel.org > > Cc: linux-doc@vger.kernel.org > > Cc: linux-ker...@vger.kernel.org > > Cc: linux-integr...@vger.kernel.org > > Cc: Randy Dunlap > > Signed-off-by: Jarkko Sakkinen > > --- > > v4: > > - Extended the text to address some of Stefan's concerns with v2. > > - Had to unfortunately remove Randy's reviewed-by because of this, given > >the amount of text added. > > v3: > > - Fixed incorrect buffer size: > > > > https://lore.kernel.org/linux-integrity/d957dbd3-4975-48d7-abc5-1a01c0959...@linux.ibm.com/ > > v2: > > - Fixed errors reported by Randy: > > > > https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ > > - Improved the text a bit to have a better presentation. > > --- > > Documentation/security/tpm/index.rst | 1 + > > Documentation/security/tpm/tpm_tis.rst | 46 ++ > > 2 files changed, 47 insertions(+) > > create mode 100644 Documentation/security/tpm/tpm_tis.rst > > > > diff --git a/Documentation/security/tpm/index.rst > > b/Documentation/security/tpm/index.rst > > index fc40e9f23c85..f27a17f60a96 100644 > > --- a/Documentation/security/tpm/index.rst > > +++ b/Documentation/security/tpm/index.rst > > @@ -5,6 +5,7 @@ Trusted Platform Module documentation > > .. toctree:: > > > > tpm_event_log > > + tpm_tis > > tpm_vtpm_proxy > > xen-tpmfront > > tpm_ftpm_tee > > diff --git a/Documentation/security/tpm/tpm_tis.rst > > b/Documentation/security/tpm/tpm_tis.rst > > new file mode 100644 > > index ..b448ea3db71d > > --- /dev/null > > +++ b/Documentation/security/tpm/tpm_tis.rst > > @@ -0,0 +1,46 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > += > > +TPM FIFO interface driver > > += > > + > > +TCG PTP Specification defines two interface types: FIFO and CRB. The > > former is > > I believe in the spec, the authors were specific to classify these as > software interfaces. Not sure if you would want to carry that > distinction into this document. > > > +based on sequenced read and write operations, and the latter is based on a > > +buffer containing the full command or response. > > + > > +FIFO (First-In-First-Out) interface is used by the tpm_tis_core dependent > > +drivers. Originally Linux had only a driver called tpm_tis, which covered > > +memory mapped (aka MMIO) interface but it was later on extended to cover > > other > > +physical interfaces supported by the TCG standard. > > Would it be worth clarifying here that one of those interfaces is > defined in the Mobile TPM specification, which also refers to its > interface as the CRB interface. In the past, this has caused great > confusion when working with individuals from the embedded community, > e.g., Arm. The Mobile TPM CRB interface, which can also be found being > used by some generations of AMD fTPM, is a doorbell style interface > using general-purpose memory. I would also point out that the Mobile TPM > CRB interface does not provide for the concept of localities. I don't necessarily disagree but it is out of scope for this. I'm not sure tho why "mobile" CRB would ever need that sort of separate dicussion. Some CRB implementations have localities some don't, and also fTPM implementations on x86 vary, no need to state that separately for mobile. > In relation to the MMIO backed interfaces, I have heard comment that the > software interfaces were not meant to require the physical interface be > MMIO. In fact, in section 9.2, "Hardware Implementation of a TPM in a PC > Client Platform", there is a comment about Locality 4 registers being > accessible via an implementation specific mechanism other than MMIO. > Additionally, there were some discussions about clarifying the PTP on > how the software interfaces might be expected to work for a > general-purpose memory backed implementation. So what is the error in the current want, i.e. what do you want to change? I think this type of stuff can be extended but I don't want to pollute this with too much detail at this point. Only what matters for daily kernel development as reminder is what this type of doc's should contain. BR, Jarkko
Re: [PATCH v4] Documentation: tpm_tis
On Sat Mar 23, 2024 at 8:40 PM EET, Jarkko Sakkinen wrote: > > Would it be worth clarifying here that one of those interfaces is > > defined in the Mobile TPM specification, which also refers to its > > interface as the CRB interface. In the past, this has caused great > > confusion when working with individuals from the embedded community, > > e.g., Arm. The Mobile TPM CRB interface, which can also be found being > > used by some generations of AMD fTPM, is a doorbell style interface > > using general-purpose memory. I would also point out that the Mobile TPM > > CRB interface does not provide for the concept of localities. > > I don't necessarily disagree but it is out of scope for this. I'm not > sure tho why "mobile" CRB would ever need that sort of separate > dicussion. > > Some CRB implementations have localities some don't, and also fTPM > implementations on x86 vary, no need to state that separately for > mobile. I.e. the variance exist but it is not "mobile" specific. E.g. when I developed tpm_crb in 2014 at that time Intel PTT only had a single locality (AFAIK later multiple localities were added to support TXT). In all cases this tpm_crb discussion is not really part of tpm_tis discussion. BR, Jarkko
[PATCH 0/2] TPM documentation updates
Re-send of TPM documentation updates. Note that in this patch set the patch versions do not have relation to the patch set version, as they were before managed as independent patches. Cc: Alexander Steffen CC: Daniel P. Smith Cc: James Bottomley Cc: Jason Gunthorpe Cc: Jonathan Corbet Cc: Lino Sanfilippo Cc: Mimi Zohar Cc: Peter Huewe Cc: Randy Dunlap Cc: linux-doc@vger.kernel.org Cc: linux-ker...@vger.kernel.org Cc: linux-integr...@vger.kernel.org v1: - Collect the latest versions of patches sent earlier. Jarkko Sakkinen (2): MAINTAINERS: Update URL's for KEYS/KEYRINGS_INTEGRITY and TPM DEVICE DRIVER Documentation: tpm_tis Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_tis.rst | 46 ++ MAINTAINERS| 3 +- 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 Documentation/security/tpm/tpm_tis.rst -- 2.44.0
[PATCH 1/2] MAINTAINERS: Update URL's for KEYS/KEYRINGS_INTEGRITY and TPM DEVICE DRIVER
Add TPM driver test suite URL to the MAINTAINERS files and move the wiki URL to more appropriate location. Link: https://gitlab.com/jarkkojs/linux-tpmdd-test Link: https://kernsec.org/wiki/index.php/Linux_Kernel_Integrity Acked-by: Paul Menzel Signed-off-by: Jarkko Sakkinen --- v2: - Typo fix: https://lore.kernel.org/all/eaa5107ac4f982b6fd6e80b522643a591e719dc9.ca...@hansenpartnership.com/ - Typo fix: https://lore.kernel.org/all/1ab10318-5e3d-417c-9984-7b17f4e38...@molgen.mpg.de/ --- MAINTAINERS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 1339918df52a..01dc4940fc06 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12015,6 +12015,7 @@ M: Mimi Zohar L: linux-integr...@vger.kernel.org L: keyri...@vger.kernel.org S: Supported +W: https://kernsec.org/wiki/index.php/Linux_Kernel_Integrity F: security/integrity/platform_certs KFENCE @@ -22344,7 +22345,7 @@ M: Jarkko Sakkinen R: Jason Gunthorpe L: linux-integr...@vger.kernel.org S: Maintained -W: https://kernsec.org/wiki/index.php/Linux_Kernel_Integrity +W: https://gitlab.com/jarkkojs/linux-tpmdd-test Q: https://patchwork.kernel.org/project/linux-integrity/list/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git F: drivers/char/tpm/ -- 2.44.0
[PATCH 2/2] Documentation: tpm_tis
Based recent discussions on LKML, provide preliminary bits of tpm_tis_core dependent drivers. Includes only bare essentials but can be extended later on case by case. This way some people may even want to read it later on. Signed-off-by: Jarkko Sakkinen --- v4: - Extended the text to address some of Stefan's concerns with v2. - Had to unfortunately remove Randy's reviewed-by because of this, given the amount of text added. v3: - Fixed incorrect buffer size: https://lore.kernel.org/linux-integrity/d957dbd3-4975-48d7-abc5-1a01c0959...@linux.ibm.com/ v2: - Fixed errors reported by Randy: https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854...@infradead.org/ - Improved the text a bit to have a better presentation. --- Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_tis.rst | 46 ++ 2 files changed, 47 insertions(+) create mode 100644 Documentation/security/tpm/tpm_tis.rst diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index fc40e9f23c85..f27a17f60a96 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -5,6 +5,7 @@ Trusted Platform Module documentation .. toctree:: tpm_event_log + tpm_tis tpm_vtpm_proxy xen-tpmfront tpm_ftpm_tee diff --git a/Documentation/security/tpm/tpm_tis.rst b/Documentation/security/tpm/tpm_tis.rst new file mode 100644 index ..b448ea3db71d --- /dev/null +++ b/Documentation/security/tpm/tpm_tis.rst @@ -0,0 +1,46 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM FIFO interface driver += + +TCG PTP Specification defines two interface types: FIFO and CRB. The former is +based on sequenced read and write operations, and the latter is based on a +buffer containing the full command or response. + +FIFO (First-In-First-Out) interface is used by the tpm_tis_core dependent +drivers. Originally Linux had only a driver called tpm_tis, which covered +memory mapped (aka MMIO) interface but it was later on extended to cover other +physical interfaces supported by the TCG standard. + +For legacy compliance the original MMIO driver is called tpm_tis and the +framework for FIFO drivers is named as tpm_tis_core. The postfix "tis" in +tpm_tis comes from the TPM Interface Specification, which is the hardware +interface specification for TPM 1.x chips. + +Communication is based on a 20 KiB buffer shared by the TPM chip through a +hardware bus or memory map, depending on the physical wiring. The buffer is +further split into five equal-size 4 KiB buffers, which provide equivalent +sets of registers for communication between the CPU and TPM. These +communication endpoints are called localities in the TCG terminology. + +When the kernel wants to send commands to the TPM chip, it first reserves +locality 0 by setting the requestUse bit in the TPM_ACCESS register. The bit is +cleared by the chip when the access is granted. Once it completes its +communication, the kernel writes the TPM_ACCESS.activeLocality bit. This +informs the chip that the locality has been relinquished. + +Pending localities are served in order by the chip in descending order, one at +a time: + +- Locality 0 has the lowest priority. +- Locality 5 has the highest priority. + +Further information on the purpose and meaning of the localities can be found +in section 3.2 of the TCG PC Client Platform TPM Profile Specification. + +References +== + +TCG PC Client Platform TPM Profile (PTP) Specification +https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/ -- 2.44.0
Re: [PATCH 2/2] Documentation: tpm_tis
On Thu Apr 11, 2024 at 1:50 PM EEST, Bagas Sanjaya wrote: > On Tue, Apr 09, 2024 at 10:08:47PM +0300, Jarkko Sakkinen wrote: > > diff --git a/Documentation/security/tpm/tpm_tis.rst > > b/Documentation/security/tpm/tpm_tis.rst > > new file mode 100644 > > index ..b448ea3db71d > > --- /dev/null > > +++ b/Documentation/security/tpm/tpm_tis.rst > > @@ -0,0 +1,46 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > += > > +TPM FIFO interface driver > > += > > + > > +TCG PTP Specification defines two interface types: FIFO and CRB. The > > former is > > +based on sequenced read and write operations, and the latter is based on a > > +buffer containing the full command or response. > > + > > +FIFO (First-In-First-Out) interface is used by the tpm_tis_core dependent > > +drivers. Originally Linux had only a driver called tpm_tis, which covered > > +memory mapped (aka MMIO) interface but it was later on extended to cover > > other > > +physical interfaces supported by the TCG standard. > > + > > +For legacy compliance the original MMIO driver is called tpm_tis and the > Did you mean "For historical reasons above ..."? That would be better wording. > > +framework for FIFO drivers is named as tpm_tis_core. The postfix "tis" in > > +tpm_tis comes from the TPM Interface Specification, which is the hardware > > +interface specification for TPM 1.x chips. > > + > > +Communication is based on a 20 KiB buffer shared by the TPM chip through a > > +hardware bus or memory map, depending on the physical wiring. The buffer is > > +further split into five equal-size 4 KiB buffers, which provide equivalent > > +sets of registers for communication between the CPU and TPM. These > > +communication endpoints are called localities in the TCG terminology. > > + > > +When the kernel wants to send commands to the TPM chip, it first reserves > > +locality 0 by setting the requestUse bit in the TPM_ACCESS register. The > > bit is > > +cleared by the chip when the access is granted. Once it completes its > > +communication, the kernel writes the TPM_ACCESS.activeLocality bit. This > > +informs the chip that the locality has been relinquished. > > + > > +Pending localities are served in order by the chip in descending order, > > one at > > +a time: > > + > > +- Locality 0 has the lowest priority. > > +- Locality 5 has the highest priority. > > + > > +Further information on the purpose and meaning of the localities can be > > found > > +in section 3.2 of the TCG PC Client Platform TPM Profile Specification. > > + > > +References > > +== > > + > > +TCG PC Client Platform TPM Profile (PTP) Specification > > +https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/ > > Other than that, > > Reviewed-by: Bagas Sanjaya Thanks! I'll apply this with the fix you proposed. For everyone: this is by no means perfect. The point is to seed something we can build on top of. So I leave it rather lacking stuff than try to document every possible bells and whistle. This can be then improved based on discussions and future patch sets. BR, Jarkko
Re: [RFC][PATCH v3 01/10] ima: Introduce hook DIGEST_LIST_CHECK
On Thu Sep 5, 2024 at 6:25 PM EEST, Roberto Sassu wrote: > From: Roberto Sassu > > Introduce a new hook to check the integrity of digest lists. "Introduce DIGEST_LIST_CHECK, a new hook..." > > The new hook is invoked during a kernel read with file type "with the file type" > READING_DIGEST LIST, which is done by the Integrity Digest Cache when it is > populating a digest cache with a digest list. The patch creates a new struct imap_rule_entry instance when it parses the corresponding rule, which means that there are couple slices of information missing here: 1. The commit message does not tell what the code change effectively is. I scavenged this information from [1]. 2. The commit message does no effort to connect the dots between the effective change and the expected goal. I'd put a lot of effort to this commit message assuming that the new hook is at the center of the goals of this patch set. [1] https://elixir.bootlin.com/linux/v6.10-rc4/source/security/integrity/ima/ima_policy.c#L1404 BR, Jarkko
Re: [RFC][PATCH v3 04/10] ima: Add digest_cache_measure/appraise boot-time built-in policies
On Thu Sep 5, 2024 at 6:25 PM EEST, Roberto Sassu wrote: > From: Roberto Sassu > > Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in > the kernel command line to add the following rule at the beginning of the > IMA policy, before other rules: > > measure func=DIGEST_LIST_CHECK pcr=12 > > which will measure digest lists into PCR 12 (or the value in > CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX). > > Specify 'digest_cache_appraise' to add the following rule at the beginning, > before other rules: > > appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig > > which will appraise digest lists with IMA signatures or module-style > appended signatures. > > Adding those rule at the beginning rather than at the end is necessary to > ensure that digest lists are measured and appraised in the initial ram > disk, which would be otherwise prevented by the dont_ rules. > > Signed-off-by: Roberto Sassu > --- > .../admin-guide/kernel-parameters.txt | 10 +- > security/integrity/ima/Kconfig| 10 ++ > security/integrity/ima/ima_policy.c | 35 +++ > 3 files changed, 54 insertions(+), 1 deletion(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt > b/Documentation/admin-guide/kernel-parameters.txt > index 09126bb8cc9f..afaaf125a237 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2077,7 +2077,8 @@ > ima_policy= [IMA] > The builtin policies to load during IMA setup. > Format: "tcb | appraise_tcb | secure_boot | > - fail_securely | critical_data" > + fail_securely | critical_data | > + digest_cache_measure | digest_cache_appraise" > > The "tcb" policy measures all programs exec'd, files > mmap'd for exec, and all files opened with the read > @@ -2099,6 +2100,13 @@ > The "critical_data" policy measures kernel integrity > critical data. > > + The "digest_cache_measure" policy measures digest lists > + into PCR 12 (can be changed with kernel config). > + > + The "digest_cache_appraise" policy appraises digest > + lists with IMA signatures or module-style appended > + signatures. > + > ima_tcb [IMA] Deprecated. Use ima_policy= instead. > Load a policy which meets the needs of the Trusted > Computing Base. This means IMA will measure all Must be updated as a separate commit as kernel-parameters.txt not part of IMA. Consider it as a different subsystem in this context. BR, Jarkko
Re: [RFC][PATCH v3 01/10] ima: Introduce hook DIGEST_LIST_CHECK
On Fri Sep 6, 2024 at 2:22 PM EEST, Roberto Sassu wrote: > On Fri, 2024-09-06 at 12:41 +0300, Jarkko Sakkinen wrote: > > On Thu Sep 5, 2024 at 6:25 PM EEST, Roberto Sassu wrote: > > > From: Roberto Sassu > > > > > > Introduce a new hook to check the integrity of digest lists. > > > > "Introduce DIGEST_LIST_CHECK, a new hook..." > > > > > > > > The new hook is invoked during a kernel read with file type > > > > "with the file type" > > > > > > > READING_DIGEST LIST, which is done by the Integrity Digest Cache when it > > > is > > > populating a digest cache with a digest list. > > > > The patch creates a new struct imap_rule_entry instance when it parses > > the corresponding rule, which means that there are couple slices of > > information missing here: > > > > 1. The commit message does not tell what the code change effectively > >is. I scavenged this information from [1]. > > Sorry, to me it seems a bit redundant to state what a IMA hook is. The > new hook will be handled by IMA like the other existing hooks. I think with documentation (scoping also to commit messages) it is in general a good strategy to put it less rather than more. No documentation is better than polluted documentation ;-) Just remarking what might not be obvious with someone who might not be obvious, unless being a pro-active contributor. BR, Jarkko
Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
On Sat, 2019-06-29 at 11:01 -0400, Sasha Levin wrote: > On Thu, Jun 27, 2019 at 02:31:35AM +0300, Jarkko Sakkinen wrote: > > On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote: > > > +static const uuid_t ftpm_ta_uuid = > > > + UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4, > > > + 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96); > > > + > > > +/** > > > + * ftpm_tee_tpm_op_recv - retrieve fTPM response. > > > + * > > > > Should not have an empty line here. > > > > > + * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h. > > > + * @buf: the buffer to store data. > > > + * @count: the number of bytes to read. > > Jarkko, w.r.t your comment above, there is an empty line between the > function name and variables in drivers/char/tpm, and in particular > tpm_crb.c which you authored and I used as reference. Do you want us to > diverge here? There is divergence and that was the first thing I've contributed to the TPM driver. I use this as the reference for formatting function descriptions these days: https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt According to that the legit way to format would be: * ftpm_tee_tpm_op_recv() - retrieve fTPM response. * @chip:the tpm_chip description as specified in driver/char/tpm/tpm.h. * @buf: the buffer to store data. * @count: the number of bytes to read. Since it is both a callback to an interface defined elsewhere and a static function and it does not document anything useful, I would just remove this comment. I'd do it for all callbacks that are part of tpm_call_ops. /Jarkko
Re: [PATCH] tpm: Document UEFI event log quirks
> +| Authors: > +| Stefan Berger I was looking how the rst formatting went from Stefan's document. This one is authored by me. /Jarkko
Re: [PATCH] tpm: Document UEFI event log quirks
On Wed, 2019-07-03 at 09:45 -0700, Randy Dunlap wrote: > > +This introduces another problem: nothing guarantees that it is not > > +called before the stub gets to run. Thus, it needs to copy the final > > +events table preboot size to the custom configuration table so that > > +kernel offset it later on. > > ? kernel can offset it later on. EFI stub calculates the total size of the events in the final events table at the time. Later on, TPM driver uses this offset to copy only the events that were actually generated after ExitBootServices(): /* * Copy any of the final events log that didn't also end up in the * main log. Events can be logged in both if events are generated * between GetEventLog() and ExitBootServices(). */ memcpy((void *)log->bios_event_log + log_size, final_tbl->events + log_tbl->final_events_preboot_size, efi_tpm_final_log_size); What would be a better way to describe this? /Jarkko
Re: [PATCH] tpm: Document UEFI event log quirks
On Wed, 2019-07-03 at 10:08 -0700, Jordan Hand wrote: > > +This introduces another problem: nothing guarantees that it is not > > +called before the stub gets to run. Thus, it needs to copy the final > > +events table preboot size to the custom configuration table so that > > +kernel offset it later on. > > This doesn't really explain what the size will be used for. Matthew's > patch description for "tpm: Don't duplicate events from the final event > log in the TCG2 log" outlines this well. You could maybe word it > differently but I think the information is necessary: > > "We can avoid this problem by looking at the size of the Final Event Log > just before we call ExitBootServices() and exporting this to the main > kernel. The kernel can then skip over all events that occured before > ExitBootServices() and only append events that were not also logged to > the main log." Not exactly sure what is missing from my paragraph. The way I see it has more information as it states what is used at as the vessel for exportation (the custom configuration table). Maybe something like: "Thus, it nees to save the final events table size at the time to the custom configuration table so that the TPM driver can later on skip the events generated during the preboot time." /Jarkko
Re: [PATCH] tpm: Document UEFI event log quirks
On Sun, 2019-07-07 at 12:33 -0700, Randy Dunlap wrote: > On 7/5/19 3:15 AM, Jarkko Sakkinen wrote: > > On Wed, 2019-07-03 at 09:45 -0700, Randy Dunlap wrote: > > > > +This introduces another problem: nothing guarantees that it is not > > > > +called before the stub gets to run. Thus, it needs to copy the final > > > > +events table preboot size to the custom configuration table so that > > > > +kernel offset it later on. > > (so that) > the kernel can use that final table preboot size as an events table > offset later on. > > > > ? kernel can offset it later on. > > > > EFI stub calculates the total size of the events in the final events > > table at the time. > > > > Later on, TPM driver uses this offset to copy only the events that > > were actually generated after ExitBootServices(): > > > > /* > > * Copy any of the final events log that didn't also end up in the > > * main log. Events can be logged in both if events are generated > > * between GetEventLog() and ExitBootServices(). > > */ > > memcpy((void *)log->bios_event_log + log_size, > >final_tbl->events + log_tbl->final_events_preboot_size, > >efi_tpm_final_log_size); > > > > What would be a better way to describe this? > > Yeah, I think I see what it's doing, how it's using that. > See above. > > OK? Your propsal looks legit, thank you. I'll send an update that tries to address yours and Jordan's feedback. /Jarkko
Re: [PATCH] tpm: Document UEFI event log quirks
On Sun, 2019-07-07 at 21:10 -0700, Jordan Hand wrote: > > "Thus, it nees to save the final events table size at the time to the > > custom configuration table so that the TPM driver can later on skip the > > events generated during the preboot time." > > > Yes, that sounds more clear to me. > > Thanks, > Jordan Awesome, thank you. /Jarkko
Re: [PATCH v8 1/2] fTPM: firmware TPM running in TEE
On Fri, Jul 05, 2019 at 04:47:45PM -0400, Sasha Levin wrote: > This patch adds support for a software-only implementation of a TPM > running in TEE. > > There is extensive documentation of the design here: > https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/ > . > > As well as reference code for the firmware available here: > https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM > > Tested-by: Thirupathaiah Annapureddy > Signed-off-by: Thirupathaiah Annapureddy > Co-authored-by: Sasha Levin > Signed-off-by: Sasha Levin Reviewed-by: Jarkko Sakkinen /Jarkko
Re: [PATCH v8 2/2] fTPM: add documentation for ftpm driver
On Fri, Jul 05, 2019 at 04:47:46PM -0400, Sasha Levin wrote: > This patch adds basic documentation to describe the new fTPM driver. > > Signed-off-by: Sasha Levin Reviewed-by: Jarkko Sakkinen /Jarkko
Re: [PATCH v8 0/2] fTPM: firmware TPM running in TEE
On Fri, Jul 05, 2019 at 04:47:44PM -0400, Sasha Levin wrote: > Changes from v7: > > - Address Jarkko's comments. > > Sasha Levin (2): > fTPM: firmware TPM running in TEE > fTPM: add documentation for ftpm driver > > Documentation/security/tpm/index.rst| 1 + > Documentation/security/tpm/tpm_ftpm_tee.rst | 27 ++ > drivers/char/tpm/Kconfig| 5 + > drivers/char/tpm/Makefile | 1 + > drivers/char/tpm/tpm_ftpm_tee.c | 350 > drivers/char/tpm/tpm_ftpm_tee.h | 40 +++ > 6 files changed, 424 insertions(+) > create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h > > -- > 2.20.1 > I applied the patches now. Appreciate a lot the patience with these. Thank you. /Jarkko
Re: [PATCH v8 0/2] fTPM: firmware TPM running in TEE
On Thu, Jul 11, 2019 at 11:10:59PM +0300, Ilias Apalodimas wrote: > Will report back any issues when we start using it on real hardware > rather than QEMU > > Thanks > /Ilias That would awesome. PR is far away so there is time to add more tested-by's. Thanks. /Jarkko
Re: [PATCH] tpm: Document UEFI event log quirks
On Mon, Jul 08, 2019 at 01:43:14PM -0700, Matthew Garrett wrote: > On Wed, Jul 3, 2019 at 9:11 AM Jarkko Sakkinen > wrote: > > +Before calling ExitBootServices() Linux EFI stub copies the event log to > > +a custom configuration table defined by the stub itself. Unfortanely, > > +the events generated by ExitBootServices() do end up to the table. > > "Unfortunately, the events generated by ExitBootServices() occur after > this and don't end up in the table"? Oops, it is a typo, thanks :-) /Jarkko
[PATCH v2] tpm: Document UEFI event log quirks
There are some weird quirks when it comes to UEFI event log. Provide a brief introduction to TPM event log mechanism and describe the quirks and how they can be sorted out. Signed-off-by: Jarkko Sakkinen --- v2: Fixed one type, adjusted the last paragraph and added the file to index.rst Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_event_log.rst | 56 2 files changed, 57 insertions(+) create mode 100644 Documentation/security/tpm/tpm_event_log.rst diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index 15783668644f..9e0815cb1e7f 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -4,5 +4,6 @@ Trusted Platform Module documentation .. toctree:: + tpm_event_log tpm_ftpm_tee tpm_vtpm_proxy diff --git a/Documentation/security/tpm/tpm_event_log.rst b/Documentation/security/tpm/tpm_event_log.rst new file mode 100644 index ..b8c39a1a3f33 --- /dev/null +++ b/Documentation/security/tpm/tpm_event_log.rst @@ -0,0 +1,56 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM Event Log += + +| Authors: +| Stefan Berger + +This document briefly describes what TPM log is and how it is handed +over from the preboot firmware to the operating system. + +Introduction + + +The preboot firmware maintains an event log that gets new entries every +time something gets hashed by it to any of the PCR registers. The events +are segregated by their type and contain the value of the hashed PCR +register. Typically, the preboot firmware will hash the components to +who execution is to be handed over or actions relevant to the boot +process. + +The main application for this is remote attestation and the reason why +it is useful is nicely put in the very first section of [1]: + +"Attestation is used to provide information about the platform???s state +to a challenger. However, PCR contents are difficult to interpret; +therefore, attestation is typically more useful when the PCR contents +are accompanied by a measurement log. While not trusted on their own, +the measurement log contains a richer set of information than do the PCR +contents. The PCR contents are used to provide the validation of the +measurement log." + +UEFI event log +== + +UEFI provided event log has a few somewhat weird quirks. + +Before calling ExitBootServices() Linux EFI stub copies the event log to +a custom configuration table defined by the stub itself. Unfortanely, +the events generated by ExitBootServices() don't end up in the table. + +The firmware provides so called final events configuration table to sort +out this issue. Events gets mirrored to this table after the first time +EFI_TCG2_PROTOCOL.GetEventLog() gets called. + +This introduces another problem: nothing guarantees that it is not called +before the Linux EFI stub gets to run. Thus, it needs to calculate and save the +final events table size while the stub is still running to the custom +configuration table so that the TPM driver can later on skip these events when +concatenating two halves of the event log from the custom configuration table +and the final events table. + +[1] +https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/ +[2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c -- 2.20.1
[PATCH v3] tpm: Document UEFI event log quirks
There are some weird quirks when it comes to UEFI event log. Provide a brief introduction to TPM event log mechanism and describe the quirks and how they can be sorted out. Signed-off-by: Jarkko Sakkinen --- v3: Add a section and use bullet list for references. Remove (invalid) author info. v2: Fixed one type, adjusted the last paragraph and added the file to index.rst Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_event_log.rst | 55 2 files changed, 56 insertions(+) create mode 100644 Documentation/security/tpm/tpm_event_log.rst diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index 15783668644f..9e0815cb1e7f 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -4,5 +4,6 @@ Trusted Platform Module documentation .. toctree:: + tpm_event_log tpm_ftpm_tee tpm_vtpm_proxy diff --git a/Documentation/security/tpm/tpm_event_log.rst b/Documentation/security/tpm/tpm_event_log.rst new file mode 100644 index ..068eeb659bb9 --- /dev/null +++ b/Documentation/security/tpm/tpm_event_log.rst @@ -0,0 +1,55 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM Event Log += + +This document briefly describes what TPM log is and how it is handed +over from the preboot firmware to the operating system. + +Introduction + + +The preboot firmware maintains an event log that gets new entries every +time something gets hashed by it to any of the PCR registers. The events +are segregated by their type and contain the value of the hashed PCR +register. Typically, the preboot firmware will hash the components to +who execution is to be handed over or actions relevant to the boot +process. + +The main application for this is remote attestation and the reason why +it is useful is nicely put in the very first section of [1]: + +"Attestation is used to provide information about the platform???s state +to a challenger. However, PCR contents are difficult to interpret; +therefore, attestation is typically more useful when the PCR contents +are accompanied by a measurement log. While not trusted on their own, +the measurement log contains a richer set of information than do the PCR +contents. The PCR contents are used to provide the validation of the +measurement log." + +UEFI event log +== + +UEFI provided event log has a few somewhat weird quirks. + +Before calling ExitBootServices() Linux EFI stub copies the event log to +a custom configuration table defined by the stub itself. Unfortanely, +the events generated by ExitBootServices() don't end up in the table. + +The firmware provides so called final events configuration table to sort +out this issue. Events gets mirrored to this table after the first time +EFI_TCG2_PROTOCOL.GetEventLog() gets called. + +This introduces another problem: nothing guarantees that it is not called +before the Linux EFI stub gets to run. Thus, it needs to calculate and save the +final events table size while the stub is still running to the custom +configuration table so that the TPM driver can later on skip these events when +concatenating two halves of the event log from the custom configuration table +and the final events table. + +References +== + +- [1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/ +- [2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c -- 2.20.1
Re: [PATCH v3] tpm: Document UEFI event log quirks
On Fri, 2019-07-12 at 07:55 -0700, Randy Dunlap wrote: > +Before calling ExitBootServices() Linux EFI stub copies the event log to > > +a custom configuration table defined by the stub itself. Unfortanely, > > [again:]Unfortunately, Ugh, I'm sorry. Sent an update. /Jarkko
[PATCH v4] tpm: Document UEFI event log quirks
There are some weird quirks when it comes to UEFI event log. Provide a brief introduction to TPM event log mechanism and describe the quirks and how they can be sorted out. Signed-off-by: Jarkko Sakkinen --- v4: - Unfortanely -> Unfortunately v3: - Add a section for refs and use a bullet list to enumerate them. - Remove an invalid author info. v2: - Fix one typo. - Refine the last paragraph to better explain how the two halves of the event log are concatenated. Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_event_log.rst | 55 2 files changed, 56 insertions(+) create mode 100644 Documentation/security/tpm/tpm_event_log.rst diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index af77a7bbb070..db566350bcd5 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -4,4 +4,5 @@ Trusted Platform Module documentation .. toctree:: + tpm_event_log tpm_vtpm_proxy diff --git a/Documentation/security/tpm/tpm_event_log.rst b/Documentation/security/tpm/tpm_event_log.rst new file mode 100644 index ..f00f7a1d5e92 --- /dev/null +++ b/Documentation/security/tpm/tpm_event_log.rst @@ -0,0 +1,55 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM Event Log += + +This document briefly describes what TPM log is and how it is handed +over from the preboot firmware to the operating system. + +Introduction + + +The preboot firmware maintains an event log that gets new entries every +time something gets hashed by it to any of the PCR registers. The events +are segregated by their type and contain the value of the hashed PCR +register. Typically, the preboot firmware will hash the components to +who execution is to be handed over or actions relevant to the boot +process. + +The main application for this is remote attestation and the reason why +it is useful is nicely put in the very first section of [1]: + +"Attestation is used to provide information about the platform???s state +to a challenger. However, PCR contents are difficult to interpret; +therefore, attestation is typically more useful when the PCR contents +are accompanied by a measurement log. While not trusted on their own, +the measurement log contains a richer set of information than do the PCR +contents. The PCR contents are used to provide the validation of the +measurement log." + +UEFI event log +== + +UEFI provided event log has a few somewhat weird quirks. + +Before calling ExitBootServices() Linux EFI stub copies the event log to +a custom configuration table defined by the stub itself. Unfortunately, +the events generated by ExitBootServices() don't end up in the table. + +The firmware provides so called final events configuration table to sort +out this issue. Events gets mirrored to this table after the first time +EFI_TCG2_PROTOCOL.GetEventLog() gets called. + +This introduces another problem: nothing guarantees that it is not called +before the Linux EFI stub gets to run. Thus, it needs to calculate and save the +final events table size while the stub is still running to the custom +configuration table so that the TPM driver can later on skip these events when +concatenating two halves of the event log from the custom configuration table +and the final events table. + +References +== + +- [1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/ +- [2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c -- 2.20.1
Re: [PATCH v8 0/2] fTPM: firmware TPM running in TEE
On Mon, Jul 15, 2019 at 12:05:25PM +0300, Ilias Apalodimas wrote: > On Fri, Jul 12, 2019 at 06:37:58AM +0300, Jarkko Sakkinen wrote: > > On Thu, Jul 11, 2019 at 11:10:59PM +0300, Ilias Apalodimas wrote: > > > Will report back any issues when we start using it on real hardware > > > rather than QEMU > > > > > > Thanks > > > /Ilias > > > > That would awesome. PR is far away so there is time to add more > > tested-by's. Thanks. > > > > I tested the basic fucntionality on QEMU and with the code only built as a > module. You can add my tested-by on this if you want Thank you. Added. /Jarkko
Re: [PATCH v4] tpm: Document UEFI event log quirks
On Wed, Jul 31, 2019 at 01:39:48PM -0600, Jonathan Corbet wrote: > On Fri, 12 Jul 2019 18:44:32 +0300 > Jarkko Sakkinen wrote: > > > There are some weird quirks when it comes to UEFI event log. Provide a > > brief introduction to TPM event log mechanism and describe the quirks > > and how they can be sorted out. > > > > Signed-off-by: Jarkko Sakkinen > > --- > > v4: - Unfortanely -> Unfortunately > > v3: - Add a section for refs and use a bullet list to enumerate them. > > - Remove an invalid author info. > > v2: - Fix one typo. > > - Refine the last paragraph to better explain how the two halves > > of the event log are concatenated. > > Documentation/security/tpm/index.rst | 1 + > > Documentation/security/tpm/tpm_event_log.rst | 55 > > 2 files changed, 56 insertions(+) > > create mode 100644 Documentation/security/tpm/tpm_event_log.rst > > I've applied this, thanks. Before I could do so, though, I had to edit > the headers, which read: > > > Content-Type: text/plain; charset=y > > "git am" *really* doesn't like "charset=y". I think this is something > that git send-email likes to do occasionally, don't know why... Thank you! /Jarkko
Re: [RFC v2 0/6] Introduce TEE based Trusted Keys support
On Tue, Jul 30, 2019 at 05:53:34PM +0530, Sumit Garg wrote: > 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 > doc: keys: Document usage of TEE based Trusted Keys > MAINTAINERS: Add entry for TEE based Trusted Keys Skimmed through the patches. I think it is better to sort out the current LKM dependency issue with trusted.ko and get TPM 1.2 and TPM 2.0 trusted keys code consolidated before it makes sense to really go detail on this. /Jarkko
Re: [PATCH v8 0/2] fTPM: firmware TPM running in TEE
On Thu, Jul 11, 2019 at 11:08:58PM +0300, Jarkko Sakkinen wrote:
> On Fri, Jul 05, 2019 at 04:47:44PM -0400, Sasha Levin wrote:
> > Changes from v7:
> >
> > - Address Jarkko's comments.
> >
> > Sasha Levin (2):
> > fTPM: firmware TPM running in TEE
> > fTPM: add documentation for ftpm driver
> >
> > Documentation/security/tpm/index.rst| 1 +
> > Documentation/security/tpm/tpm_ftpm_tee.rst | 27 ++
> > drivers/char/tpm/Kconfig| 5 +
> > drivers/char/tpm/Makefile | 1 +
> > drivers/char/tpm/tpm_ftpm_tee.c | 350
> > drivers/char/tpm/tpm_ftpm_tee.h | 40 +++
> > 6 files changed, 424 insertions(+)
> > create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst
> > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c
> > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h
> >
> > --
> > 2.20.1
> >
>
> I applied the patches now. Appreciate a lot the patience with these.
> Thank you.
Hi, can you possibly fix these:
005-tpm-tpm_ftpm_tee-A-driver-for-firmware-TPM-running-i.patch
---
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per
line)
#10:
https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/
.
WARNING: Non-standard signature: Co-authored-by:
#18:
Co-authored-by: Sasha Levin
WARNING: prefer 'help' over '---help---' for new help texts
#39: FILE: drivers/char/tpm/Kconfig:167:
+config TCG_FTPM_TEE
WARNING: please write a paragraph that describes the config symbol fully
#39: FILE: drivers/char/tpm/Kconfig:167:
+config TCG_FTPM_TEE
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#57:
new file mode 100644
WARNING: please, no space before tabs
#102: FILE: drivers/char/tpm/tpm_ftpm_tee.c:41:
+ * ^IIn case of success the number of bytes received.$
WARNING: please, no space before tabs
#131: FILE: drivers/char/tpm/tpm_ftpm_tee.c:70:
+ * ^IIn case of success, returns 0.$
WARNING: please, no space before tabs
#276: FILE: drivers/char/tpm/tpm_ftpm_tee.c:215:
+ * ^IOn success, 0. On failure, -errno.$
WARNING: please, no space before tabs
#366: FILE: drivers/char/tpm/tpm_ftpm_tee.c:305:
+ * ^I0 always.$
ERROR: code indent should use tabs where possible
#387: FILE: drivers/char/tpm/tpm_ftpm_tee.c:326:
+/* memory allocated with devm_kzalloc() is freed automatically */$
WARNING: DT compatible string "microsoft,ftpm" appears un-documented -- check
./Documentation/devicetree/bindings/
#393: FILE: drivers/char/tpm/tpm_ftpm_tee.c:332:
+ { .compatible = "microsoft,ftpm" },
WARNING: DT compatible string vendor "microsoft" appears un-documented -- check
./Documentation/devicetree/bindings/vendor-prefixes.yaml
#393: FILE: drivers/char/tpm/tpm_ftpm_tee.c:332:
+ { .compatible = "microsoft,ftpm" },
total: 1 errors, 11 warnings, 405 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
NOTE: Whitespace errors detected.
You may wish to use scripts/cleanpatch or scripts/cleanfile
I temporarily dropped the patches but can apply them once the issues
are fixed.
/Jarkko
Re: [PATCH v8 0/2] fTPM: firmware TPM running in TEE
On Mon, Aug 05, 2019 at 02:05:18PM -0400, Sasha Levin wrote: > On Mon, Aug 05, 2019 at 12:44:28AM +0300, Jarkko Sakkinen wrote: > > On Thu, Jul 11, 2019 at 11:08:58PM +0300, Jarkko Sakkinen wrote: > > > On Fri, Jul 05, 2019 at 04:47:44PM -0400, Sasha Levin wrote: > > > > Changes from v7: > > > > > > > > - Address Jarkko's comments. > > > > > > > > Sasha Levin (2): > > > > fTPM: firmware TPM running in TEE > > > > fTPM: add documentation for ftpm driver > > > > > > > > Documentation/security/tpm/index.rst| 1 + > > > > Documentation/security/tpm/tpm_ftpm_tee.rst | 27 ++ > > > > drivers/char/tpm/Kconfig| 5 + > > > > drivers/char/tpm/Makefile | 1 + > > > > drivers/char/tpm/tpm_ftpm_tee.c | 350 > > > > drivers/char/tpm/tpm_ftpm_tee.h | 40 +++ > > > > 6 files changed, 424 insertions(+) > > > > create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst > > > > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c > > > > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h > > > > > > > > -- > > > > 2.20.1 > > > > > > > > > > I applied the patches now. Appreciate a lot the patience with these. > > > Thank you. > > > > Hi, can you possibly fix these: > > Any objection to sending you a patch on top of your tree instead? Go ahead. Added the previous patches to my master. /Jarkko
Re: [PATCH] ftpm: add shutdown call back
On Fri, Oct 11, 2019 at 10:57:21AM -0400, Pavel Tatashin wrote:
> From: thiruan
>
> add shutdown call back to close existing session with fTPM TA
> to support kexec scenario.
>
> Signed-off-by: Thirupathaiah Annapureddy
> Signed-off-by: Pavel Tatashin
Use the correct tag in the short summary (tpm/tpm_ftpm_tee).
> ---
> drivers/char/tpm/tpm_ftpm_tee.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 6640a14dbe48..c245be6f4015 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -328,6 +328,27 @@ static int ftpm_tee_remove(struct platform_device *pdev)
> return 0;
> }
>
> +/**
> + * ftpm_tee_shutdown - shutdown the TPM device
> + * @pdev: the platform_device description.
> + *
> + * Return:
> + * none.
Do not document return values for a void function. The last three lines
do not serve any purpose.
> + */
> +static void ftpm_tee_shutdown(struct platform_device *pdev)
> +{
> + struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
> +
> + /* Free the shared memory pool */
> + tee_shm_free(pvt_data->shm);
Is it unexpected that calling tee_shm_free() free's a shared memory
pool? A comment here implies exactly that.
> + /* close the existing session with fTPM TA*/
> + tee_client_close_session(pvt_data->ctx, pvt_data->session);
Ditto.
> +
> + /* close the context with TEE driver */
> + tee_client_close_context(pvt_data->ctx);
Ditto.
> +}
> +
> static const struct of_device_id of_ftpm_tee_ids[] = {
> { .compatible = "microsoft,ftpm" },
> { }
> @@ -341,6 +362,7 @@ static struct platform_driver ftpm_tee_driver = {
> },
> .probe = ftpm_tee_probe,
> .remove = ftpm_tee_remove,
> + .shutdown = ftpm_tee_shutdown,
> };
>
> module_platform_driver(ftpm_tee_driver);
> --
> 2.23.0
>
/Jarkko
Re: [PATCH v2] tpm/tpm_ftpm_tee: add shutdown call back
On Mon, Oct 14, 2019 at 04:21:35PM -0400, Pavel Tatashin wrote: > add shutdown call back to close existing session with fTPM TA > to support kexec scenario. Sentences start in English with a capital letter :-) > > Signed-off-by: Thirupathaiah Annapureddy > Signed-off-by: Pavel Tatashin > --- > drivers/char/tpm/tpm_ftpm_tee.c | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c > index 6640a14dbe48..ad16ea555e97 100644 > --- a/drivers/char/tpm/tpm_ftpm_tee.c > +++ b/drivers/char/tpm/tpm_ftpm_tee.c > @@ -328,6 +328,19 @@ static int ftpm_tee_remove(struct platform_device *pdev) > return 0; > } > > +/** > + * ftpm_tee_shutdown - shutdown the TPM device A function name has to have parentheses in kdoc. I know this not consistently done in the subsystem ATM but this is what is documented here: https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt /Jarkko
Re: [PATCH v3] tpm/tpm_ftpm_tee: add shutdown call back
On Wed, Oct 16, 2019 at 12:31:14PM -0400, Pavel Tatashin wrote: > Add shutdown call back to close existing session with fTPM TA > to support kexec scenario. > > Add parentheses to function names in comments as specified in kdoc. > > Signed-off-by: Thirupathaiah Annapureddy > Signed-off-by: Pavel Tatashin LGTM Reviewed-by: Jarkko Sakkinen I have no means to test this though. It still needs a tested-by. /Jarkko
Re: [PATCH v3 1/2] ftpm: firmware TPM running in TEE
On Mon, Apr 15, 2019 at 11:56:35AM -0400, Sasha Levin wrote:
> This patch adds support for a software-only implementation of a TPM
> running in TEE.
>
> There is extensive documentation of the design here:
> https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/
> .
>
> As well as reference code for the firmware available here:
> https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM
The commit message should include at least a brief description what TEE
is.
>
> Signed-off-by: Thirupathaiah Annapureddy
> Signed-off-by: Sasha Levin
> ---
> drivers/char/tpm/Kconfig| 5 +
> drivers/char/tpm/Makefile | 1 +
> drivers/char/tpm/tpm_ftpm_tee.c | 366
> drivers/char/tpm/tpm_ftpm_tee.h | 47
> 4 files changed, 419 insertions(+)
> create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c
> create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 536e55d3919f..5638726641eb 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -164,6 +164,11 @@ config TCG_VTPM_PROXY
> /dev/vtpmX and a server-side file descriptor on which the vTPM
> can receive commands.
>
> +config TCG_FTPM_TEE
> + tristate "TEE based fTPM Interface"
> + depends on TEE
> + ---help---
> + This driver proxies for fTPM running in TEE
>
> source "drivers/char/tpm/st33zp24/Kconfig"
> endif # TCG_TPM
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a01c4cab902a..c354cdff9c62 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
> obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
> obj-$(CONFIG_TCG_CRB) += tpm_crb.o
> obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> +obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> new file mode 100644
> index ..f33cdfeb5376
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -0,0 +1,366 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation
> + */
There should be at least some description what kind of implementation
this is and something about TEE.
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "tpm.h"
> +#include "tpm_ftpm_tee.h"
> +
> +#define DRIVER_NAME "ftpm-tee"
> +
> +/* TA_FTPM_UUID: BC50D971-D4C9-42C4-82CB-343FB7F37896 */
> +static const uuid_t ftpm_ta_uuid =
> + UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
> + 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);
Just wondering why prefixes are here in different order in the comment
and code.
> +
> +/*
> + * Note: ftpm_tee_tpm_op_recv and ftpm_tee_tpm_op_send are called from the
> + * same routine tpm_try_transmit in tpm-interface.c. These calls are
> protected
> + * by chip->tpm_mutex => There is no need for protecting any data shared
> + * between these routines ex: struct ftpm_tee_private
> + */
This documentation block should be removed. It could be in all drivers
and thus this documentation belongs outside of specific HW drivers.
> +
> +/**
> + * ftpm_tee_tpm_op_recv retrieve fTPM response.
> + * @param: chip, the tpm_chip description as specified in
> driver/char/tpm/tpm.h.
> + * @param: buf, the buffer to store data.
> + * @param: count, the number of bytes to read.
> + * @return: In case of success the number of bytes received.
> + * In other case, a < 0 value describing the issue.
> + */
This should be modified to follow kdoc [1]. It is inconsistent with the
other kdoc comments we have. Now it is all wrong and contains redundant
information. It should be something like
/**
* ftpm_tee_tpm_op_recv() - retrieve a response from fTPM
* @chip: TPM chip associated with the fTPM
* @buf:buffer for the received data
* @count: number of bytes to read
*
* Copy the response from fTPM's internal buffer to the buffer provided
* by the caller.
*
* Return:
* 0 on success,
* -errno on error
*/
> +static int ftpm_tee_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent);
> + size_t len;
> +
> + len = pvt_data->resp_len;
> + if (count < len) {
> + dev_err(&chip->dev,
> + "%s:Invalid size in recv: count=%zd, resp_len=%zd\n",
> + __func__, count, len);
> + return -EIO;
> + }
> +
> + memcpy(buf, pvt_data->resp_buf, len);
> + pvt_data->resp_len = 0;
> +
> + return len;
> +}
> +
> +/**
> + * ftpm_tee_tpm_op_send send TPM commands through the TEE shared memory.
> + *
> + * @param: chip, the tpm_chip description as specified in
> driver/char/tpm/tpm.h
> + * @param: buf, the buffer t
Re: [PATCH v3 2/2] ftpm: add documentation for ftpm driver
On Mon, Apr 15, 2019 at 11:56:36AM -0400, Sasha Levin wrote: > This patch adds basic documentation to describe the new fTPM driver. > > Signed-off-by: Sasha Levin > Signed-off-by: Sasha Levin (Microsoft) > --- > Documentation/security/tpm/index.rst| 1 + > Documentation/security/tpm/tpm_ftpm_tee.rst | 31 + > 2 files changed, 32 insertions(+) > create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst > > diff --git a/Documentation/security/tpm/index.rst > b/Documentation/security/tpm/index.rst > index af77a7bbb070..15783668644f 100644 > --- a/Documentation/security/tpm/index.rst > +++ b/Documentation/security/tpm/index.rst > @@ -4,4 +4,5 @@ Trusted Platform Module documentation > > .. toctree:: > > + tpm_ftpm_tee > tpm_vtpm_proxy > diff --git a/Documentation/security/tpm/tpm_ftpm_tee.rst > b/Documentation/security/tpm/tpm_ftpm_tee.rst > new file mode 100644 > index ..29c2f8b5ed10 > --- /dev/null > +++ b/Documentation/security/tpm/tpm_ftpm_tee.rst > @@ -0,0 +1,31 @@ > += > +Firmware TPM Driver > += > + > +| Authors: > +| Thirupathaiah Annapureddy > +| Sasha Levin > + > +This document describes the firmware Trusted Platform Module (fTPM) > +device driver. > + > +Introduction > + > + > +This driver is a shim for a firmware implemented in ARM's TrustZone > +environment. The driver allows programs to interact with the TPM in the same > +way the would interact with a hardware TPM. > + > +Design > +== > + > +The driver acts as a thin layer that passes commands to and from a TPM > +implemented in firmware. The driver itself doesn't contain much logic and is > +used more like a dumb pipe between firmware and kernel/userspace. > + > +The firmware itself is based on the following paper: > +https://www.microsoft.com/en-us/research/wp-content/uploads/2017/06/ftpm1.pdf > + > +When the driver is loaded it will expose ``/dev/tpmX`` character devices to > +userspace which will enable userspace to communicate with the firmware tpm > +through this device. > -- > 2.19.1 > Actually this would a better place at least with some words to describe what is TEE. I'm, for example, confused whether there is only single TEE in existence always used with TZ or is this some MS specific TEE. Otherwise, looks legit. /Jarkko
Re: [PATCH v3 1/2] ftpm: firmware TPM running in TEE
On Fri, May 17, 2019 at 09:22:26AM -0400, Sasha Levin wrote: > The whole TEE subsystem is already well documented in our kernel tree > (https://www.kernel.org/doc/Documentation/tee.txt) and beyond. I can add > a reference to the doc here, but I'd rather not add a bunch of TEE > related comments as you suggest later in your review. > > The same way a PCI device driver doesn't describe what PCI is in it's > code, we shouldn't be doing the same for TEE here. Thanks for pointing out the documentation! That is sufficient. /Jarkko
Re: [PATCH v4 0/2] fTPM: firmware TPM running in TEE
On Thu, May 30, 2019 at 11:27:56AM -0400, Sasha Levin wrote: > Changes since v3: > > - Address comments by Jarkko Sakkinen > - Address comments by Igor Opaniuk > > Sasha Levin (2): > fTPM: firmware TPM running in TEE > fTPM: add documentation for ftpm driver I think patches start to look proper but I wonder can anyone test these? I don't think before that I can merge these. /Jarkko
Re: [PATCH 18/22] docs: security: trusted-encrypted.rst: fix code-block tag
On Wed, May 29, 2019 at 08:23:49PM -0300, Mauro Carvalho Chehab wrote: > The code-block tag is at the wrong place, causing those > warnings: > > Documentation/security/keys/trusted-encrypted.rst:112: WARNING: Literal > block expected; none found. > Documentation/security/keys/trusted-encrypted.rst:121: WARNING: > Unexpected indentation. > Documentation/security/keys/trusted-encrypted.rst:122: WARNING: Block > quote ends without a blank line; unexpected unindent. > Documentation/security/keys/trusted-encrypted.rst:123: WARNING: Block > quote ends without a blank line; unexpected unindent. > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Jarkko Sakkinen /Jarkko
Re: [PATCH v4 0/2] fTPM: firmware TPM running in TEE
On Mon, Jun 03, 2019 at 05:16:48PM -0400, Sasha Levin wrote: > On Mon, Jun 03, 2019 at 11:28:15PM +0300, Jarkko Sakkinen wrote: > > On Thu, May 30, 2019 at 11:27:56AM -0400, Sasha Levin wrote: > > > Changes since v3: > > > > > > - Address comments by Jarkko Sakkinen > > > - Address comments by Igor Opaniuk > > > > > > Sasha Levin (2): > > > fTPM: firmware TPM running in TEE > > > fTPM: add documentation for ftpm driver > > > > I think patches start to look proper but I wonder can anyone test > > these? I don't think before that I can merge these. > > They're all functionally tested by us on actual hardware before being > sent out. > > The reference implementation is open and being kept updated, and an > interested third party should be able to verify the correctness of these > patches. However, it doesn't look like there's an interested third party > given that these patches have been out for a few months now. So can they be tagged with your tested-by? /Jarkko
Re: [PATCH v4 1/2] fTPM: firmware TPM running in TEE
On Tue, Jun 04, 2019 at 11:45:52AM +0530, Sumit Garg wrote: > Is this well tested? I see this misleading error multiple times as > follows although TEE driver works pretty well. > > Module built with "CONFIG_TCG_FTPM_TEE=y" > > [1.436878] ftpm-tee tpm@0: ftpm_tee_probe:tee_client_open_context failed > [1.509471] ftpm-tee tpm@0: ftpm_tee_probe:tee_client_open_context failed > [1.517268] ftpm-tee tpm@0: ftpm_tee_probe:tee_client_open_context failed > [1.525596] ftpm-tee tpm@0: ftpm_tee_probe:tee_client_open_context failed > > -Sumit No testing done from my part. /Jarkko
Re: [RFC 1/7] tee: optee: allow kernel pages to register as shm
On Thu, Jun 13, 2019 at 04:00:27PM +0530, Sumit Garg wrote: > 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 Just out of pure interest why this was not allowed before? /Jarkko
Re: [RFC 1/7] tee: optee: allow kernel pages to register as shm
On Thu, Jun 13, 2019 at 06:12:57PM +0300, Jarkko Sakkinen wrote: > On Thu, Jun 13, 2019 at 04:00:27PM +0530, Sumit Garg wrote: > > 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 > > Just out of pure interest why this was not allowed before? Please spare me and ignore that one :-) Obviouslly because it was not used. Acked-by: Jarkko Sakkinen /Jarkko
Re: [RFC 1/7] tee: optee: allow kernel pages to register as shm
On Thu, Jun 13, 2019 at 06:17:14PM +0300, Jarkko Sakkinen wrote: > On Thu, Jun 13, 2019 at 06:12:57PM +0300, Jarkko Sakkinen wrote: > > On Thu, Jun 13, 2019 at 04:00:27PM +0530, Sumit Garg wrote: > > > 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 > > > > Just out of pure interest why this was not allowed before? > > Please spare me and ignore that one :-) Obviouslly because it > was not used. > > Acked-by: Jarkko Sakkinen Actually, Reviewed-by: Jarkko Sakkinen /Jarkko
Re: [RFC 2/7] tee: enable support to register kernel memory
On Thu, Jun 13, 2019 at 04:00:28PM +0530, Sumit Garg wrote: > 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 Reviewed-by: Jarkko Sakkinen /Jarkko
Re: [RFC 4/7] KEYS: trusted: Introduce TEE based Trusted Keys
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 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
Re: [RFC 6/7] doc: keys: Document usage of TEE based Trusted Keys
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 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
Re: [RFC 6/7] doc: keys: Document usage of TEE based Trusted Keys
On Fri, Jun 14, 2019 at 11:07:23AM +0530, Sumit Garg wrote: > On Thu, 13 Jun 2019 at 21:04, Jarkko Sakkinen > wrote: > > > > 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 > > > > 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. > > > > I understand your point. > > When I initially looked at trusted key implementation, it seemed to be > tightly coupled to use TPM device. So I implemented a parallel > implementation to get initial feedback (functionality-wise) on this > new approach. Yeah, I completely get this. My feedback this is: we can definitely consider TEE based trusted keys, and I know that trusted.ko is a mess, but still that is the only right long-term path. Think about the positive side: if you as a side-effect can make it cleaner and more versatile, your patch set will improve the quality of the kernel as a whole i.e. you benefit larger audience than just TEE user base :-) > I will work on abstraction of trusted key apis to use either approach. > But is it fine with you if I send if I send a separate RFC patch for > abstraction and later once reviewed I will incorporate that patch in > this patch-set. > > It will be really helpful if you could help to test that abstraction > patch with a real TPM device as I doesn't posses one to test. I can, yes. /Jarkko
On Nitrokey Pro's ECC support
Hi I was getting myself a smartcard stick and looked for options from [1]. The documentation says that Nitrokey Pro does not support ECC while it actually does [2]. I was already canceling my order when Jan Suhr, the CEO of that company, kindly pointed out to me this. [1] https://www.kernel.org/doc/html/latest/process/maintainer-pgp-guide.html [2] https://shop.nitrokey.com/shop/product/nitrokey-pro-2-3 /Jarkko signature.asc Description: This is a digitally signed message part
Re: On Nitrokey Pro's ECC support
On 2019-06-26 18:28, Jonathan Corbet wrote: On Wed, 26 Jun 2019 11:21:38 -0400 Konstantin Ryabitsev wrote: >Maybe Konstantin (copied) might be willing to supply an update to the >document to reflect this? Hello: I just sent a patch with updates that reflect ECC capabilities in newer devices. Hey, man, that took you just under an hour to get done. We can't all just wait around while you twiddle your thumbs... :) Seriously, though, thanks for doing this, jon +1 :-) /Jarkko
Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote:
> This patch adds support for a software-only implementation of a TPM
> running in TEE.
>
> There is extensive documentation of the design here:
>
https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/
> .
>
> As well as reference code for the firmware available here:
>
https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM
>
> Tested-by: Thirupathaiah Annapureddy
> Signed-off-by: Thirupathaiah Annapureddy
> Signed-off-by: Sasha Levin
You've used so much on this so shouldn't this have that somewhat new
co-developed-by tag? I'm also wondering can this work at all
process-wise if the original author of the patch is also the only tester
of the patch?
> ---
> drivers/char/tpm/Kconfig| 5 +
> drivers/char/tpm/Makefile | 1 +
> drivers/char/tpm/tpm_ftpm_tee.c | 356
> drivers/char/tpm/tpm_ftpm_tee.h | 40
> 4 files changed, 402 insertions(+)
> create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c
> create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 88a3c06fc153..17bfbf9f572f 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -164,6 +164,11 @@ config TCG_VTPM_PROXY
> /dev/vtpmX and a server-side file descriptor on which the vTPM
> can receive commands.
>
> +config TCG_FTPM_TEE
> + tristate "TEE based fTPM Interface"
> + depends on TEE && OPTEE
> + ---help---
> + This driver proxies for firmware TPM running in TEE.
>
> source "drivers/char/tpm/st33zp24/Kconfig"
> endif # TCG_TPM
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a01c4cab902a..c354cdff9c62 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
> obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
> obj-$(CONFIG_TCG_CRB) += tpm_crb.o
> obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> +obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> new file mode 100644
> index ..0312c10767bd
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -0,0 +1,356 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation
The statement does not contain the years. I'm also wondering in more
generic sense that with Git, which in its inner structure contains all
the metadata to deriving this type of information, is this more like a
legacy thing or why should be put these statements to new files?
> + *
> + * Implements a firmware TPM as described here:
> + *
>
https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/
> + *
> + * A reference implementation is available here:
> + *
>
https://github.com/microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM/optee_ta/fTPM
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "tpm.h"
> +#include "tpm_ftpm_tee.h"
> +
> +#define DRIVER_NAME "ftpm-tee"
Should be open coded where it is used because this does not bring any
practical value.
> +
> +/*
> + * TA_FTPM_UUID: BC50D971-D4C9-42C4-82CB-343FB7F37896
> + *
> + * Randomly generated, and must correspond to the GUID on the TA side.
> + * Defined here in the reference implementation:
> + *
>
https://github.com/microsoft/ms-tpm-20-ref/blob/master/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/include/fTPM.h#L42
> + */
> +
Probably should delete this empty line.
> +static const uuid_t ftpm_ta_uuid =
> + UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
> + 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);
> +
> +/**
> + * ftpm_tee_tpm_op_recv - retrieve fTPM response.
> + *
Should not have an empty line here.
> + * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h.
> + * @buf: the buffer to store data.
> + * @count: the number of bytes to read.
Should be aligned with a tab character.
> + *
> + * Return:
> + * In case of success the number of bytes received.
> + * On failure, -errno.
> + */
> +static int ftpm_tee_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent);
> + size_t len;
> +
> + len = pvt_data->resp_len;
> + if (count < len) {
> + dev_err(&chip->dev,
> + "%s:Invalid size in recv: count=%zd, resp_len=%zd\n",
^ a single white space also here
> + __func__, count, len);
> + return -EIO;
> + }
> +
> + memcpy(buf, pvt_data->resp_buf, len);
> + pvt_data->resp_len = 0;
> +
> + return len;
> +}
> +
> +/**
> + * ftpm_tee_tpm_op_send - send TPM commands through the TEE shared memory.
> + *
Sh
Re: [PATCH v7 2/2] fTPM: add documentation for ftpm driver
On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote: > This patch adds basic documentation to describe the new fTPM driver. > > Signed-off-by: Sasha Levin > --- > Documentation/security/tpm/index.rst| 1 + > Documentation/security/tpm/tpm_ftpm_tee.rst | 31 + > 2 files changed, 32 insertions(+) > create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst > > diff --git a/Documentation/security/tpm/index.rst > b/Documentation/security/tpm/index.rst > index af77a7bbb070..15783668644f 100644 > --- a/Documentation/security/tpm/index.rst > +++ b/Documentation/security/tpm/index.rst > @@ -4,4 +4,5 @@ Trusted Platform Module documentation > > .. toctree:: > > + tpm_ftpm_tee > tpm_vtpm_proxy > diff --git a/Documentation/security/tpm/tpm_ftpm_tee.rst > b/Documentation/security/tpm/tpm_ftpm_tee.rst > new file mode 100644 > index ..48de0dcec0f6 > --- /dev/null > +++ b/Documentation/security/tpm/tpm_ftpm_tee.rst > @@ -0,0 +1,31 @@ > += > +Firmware TPM Driver > += > + > +| Authors: > +| Thirupathaiah Annapureddy > +| Sasha Levin The way I see it this thing should not exist here at all but the first patch should have co-developed-by in addition to signed-off-by from you. Otherwise looks good. The list of authors is againt something that Git does better than humans. /Jarkko
Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote: > > You've used so much on this so shouldn't this have that somewhat new > > co-developed-by tag? I'm also wondering can this work at all > > Honestly, I've just been massaging this patch more than "authoring" it. > If you feel strongly about it feel free to add a Co-authored patch with > my name, but in my mind this is just Thiru's work. This is just my subjective view but writing code is easier than making it work in the mainline in 99% of cases. If this patch was doing something revolutional, lets say a new outstanding scheduling algorithm, then I would think otherwise. It is not. You without question deserve both credit and also the blame (if this breaks everything) :-) > > process-wise if the original author of the patch is also the only tester > > of the patch? > > There's not much we can do about this... Linaro folks have tested this > without the fTPM firmware, so at the very least it won't explode for > everyone. If for some reason non-microsoft folks see issues then we can > submit patches on top to fix this, we're not just throwing this at you > and running away. So why any of those Linaro folks can't do it? I can add after tested-by tag parentheses something explaining that context of testing. It is reasonable given the circumstances. I can also give an explanation in my next PR along the lines what you are saying. This would definitely work for me. /Jarkko
Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
On Thu, 2019-06-27 at 16:17 +0300, Jarkko Sakkinen wrote: > On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote: > > > You've used so much on this so shouldn't this have that somewhat new > > > co-developed-by tag? I'm also wondering can this work at all > > > > Honestly, I've just been massaging this patch more than "authoring" it. > > If you feel strongly about it feel free to add a Co-authored patch with > > my name, but in my mind this is just Thiru's work. > > This is just my subjective view but writing code is easier than making > it work in the mainline in 99% of cases. If this patch was doing > something revolutional, lets say a new outstanding scheduling algorithm, > then I would think otherwise. It is not. You without question deserve > both credit and also the blame (if this breaks everything) :-) Not like I'm putting the pressure on this. You make the call with the tag. Put it if you wwant. I'm cool with either. /Jarkko
Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
On Thu, 2019-06-27 at 16:30 +0300, Ilias Apalodimas wrote: > is really useful. I don't have hardware to test this at the moment, but once i > get it, i'll give it a spin. Thank you for responding, really appreciate it. Please note, however, that I already did my v5.3 PR so there is a lot of time to give it a spin. In all cases, we will find a way to put this to my v5.4 PR. I don't see any reason why not. As soon as the cosmetic stuff is fixed that I remarked in v7 I'm ready to take this to my tree and after that soonish make it available on linux-next. /Jarkko
[PATCH] tpm: Document UEFI event log quirks
There are some weird quirks when it comes to UEFI event log. Provide a brief introduction to TPM event log mechanism and describe the quirks and how they can be sorted out. Signed-off-by: Jarkko Sakkinen --- Documentation/security/tpm/tpm-eventlog.rst | 53 + 1 file changed, 53 insertions(+) create mode 100644 Documentation/security/tpm/tpm-eventlog.rst diff --git a/Documentation/security/tpm/tpm-eventlog.rst b/Documentation/security/tpm/tpm-eventlog.rst new file mode 100644 index ..2ca8042bdb17 --- /dev/null +++ b/Documentation/security/tpm/tpm-eventlog.rst @@ -0,0 +1,53 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +TPM Event Log += + +| Authors: +| Stefan Berger + +This document briefly describes what TPM log is and how it is handed +over from the preboot firmware to the operating system. + +Introduction + + +The preboot firmware maintains an event log that gets new entries every +time something gets hashed by it to any of the PCR registers. The events +are segregated by their type and contain the value of the hashed PCR +register. Typically, the preboot firmware will hash the components to +who execution is to be handed over or actions relevant to the boot +process. + +The main application for this is remote attestation and the reason why +it is useful is nicely put in the very first section of [1]: + +"Attestation is used to provide information about the platform???s state +to a challenger. However, PCR contents are difficult to interpret; +therefore, attestation is typically more useful when the PCR contents +are accompanied by a measurement log. While not trusted on their own, +the measurement log contains a richer set of information than do the PCR +contents. The PCR contents are used to provide the validation of the +measurement log." + +UEFI event log +== + +UEFI provided event log has a few somewhat weird quirks. + +Before calling ExitBootServices() Linux EFI stub copies the event log to +a custom configuration table defined by the stub itself. Unfortanely, +the events generated by ExitBootServices() do end up to the table. + +The firmware provides so called final events configuration table to sort +out this issue. Events gets mirrored to this table after the first time +EFI_TCG2_PROTOCOL.GetEventLog() gets called. + +This introduces another problem: nothing guarantees that it is not +called before the stub gets to run. Thus, it needs to copy the final +events table preboot size to the custom configuration table so that +kernel offset it later on. + +[1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/ +[2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c -- 2.20.1
Re: [PATCH v5 11/11] intel_sgx: driver documentation
On Fri, Nov 17, 2017 at 01:43:10PM -0800, Darren Hart wrote: > This series will need to be updated per the comments received so far, as > well as with commit messages and a complete Cc list *per patch* giving > all required parties an opportunity to review. > > With respect to the obvious security nature of this series, who from the > kernel security folks are going to be reviewing this? > secur...@kernel.org? I think it would make sense to CC this to linux-security module. > Cc updated for this thread, and specifically the question regarding > location below: > > > Signed-off-by: Jarkko Sakkinen > > --- > > Documentation/index.rst | 1 + > > Documentation/x86/intel_sgx.rst | 131 > > > > 2 files changed, 132 insertions(+) > > create mode 100644 Documentation/x86/intel_sgx.rst > > > > ... > > > diff --git a/Documentation/x86/intel_sgx.rst > > b/Documentation/x86/intel_sgx.rst > > new file mode 100644 > > index ..34bcf6a2a495 > > --- /dev/null > > +++ b/Documentation/x86/intel_sgx.rst > > @@ -0,0 +1,131 @@ > > +=== > > +Intel(R) SGX driver > > +=== > > + > > +Introduction > > + > > + > > +Intel(R) SGX is a set of CPU instructions that can be used by applications > > to > > +set aside private regions of code and data. The code outside the enclave is > > +disallowed to access the memory inside the enclave by the CPU access > > control. > > +In a way you can think that SGX provides inverted sandbox. It protects the > > +application from a malicious host. > > + > > +There is a new hardware unit in the processor called Memory Encryption > > Engine > > +(MEE) starting from the Skylake microarchitecture. BIOS can define one or > > many > > +MEE regions that can hold enclave data by configuring them with PRMRR > > registers. > > + > > +The MEE automatically encrypts the data leaving the processor package to > > the MEE > > +regions. The data is encrypted using a random key whose life-time is > > exactly one > > +power cycle. > > + > > +You can tell if your CPU supports SGX by looking into ``/proc/cpuinfo``: > > + > > + ``cat /proc/cpuinfo | grep sgx`` > > Is SGX considered architectural or not? A quick search of the SDM > includes it in Volume 3: > > Volume 3: Includes the full system programming guide, parts 1, 2, 3, and > 4. Describes the operating-system support environment of Intel® 64 and > IA-32 architectures, including: memory management, protection, task > management, interrupt and exception handling, multi-processor support, > thermal and power management features, debugging, performance > monitoring, system management mode, virtual machine extensions (VMX) > instructions, Intel® Virtualization Technology (Intel® VT), and Intel® > Software Guard Extensions (Intel® SGX). > > https://software.intel.com/en-us/articles/intel-sdm > > Depending on the answer, this impacts whether this belongs in > drivers/platform/x86 or arch/x86/platform per our recent agreement with > Thomas. > > Thomas, Mingo, HPA, do you wish to see this organized/located > differently than it is here in v5? The code is made easily relocatable. I just wanted to keep it as an encapsulated driver up until I hear the maintainer feedback. I'll submit v6 with code otherwise fixed according to the feedback that I've heard up until that point and relocate it in v7 based on your feedback. > > +Launch control > > +== > > + > > +For launching an enclave, two structures must be provided for ENCLS(EINIT): > > + > > +1. **SIGSTRUCT:** a signed measurement of the enclave binary. > > +2. **EINITTOKEN:** the measurement, the public key of the signer and > > various > > + enclave attributes. This structure contains a MAC of its contents using > > + hardware derived symmetric key called *launch key*. > > + > > +The hardware platform contains a root key pair for signing the SIGTRUCT > > +for a *launch enclave* that is able to acquire the *launch key* for > > +creating EINITTOKEN's for other enclaves. For the launch enclave > > +EINITTOKEN is not needed because it is signed with the private root key. > > + > > +There are two feature control bits associate with launch control > > Nit: missing colon at the end of the line above ^ Yes. Thanks for spotting that out :-) /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 00/11] Intel SGX Driver
NGE and not just page aligned. * Require kernel to be compiled with X64_64 and CPU_SUP_INTEL. * Fixed an incorrect value for SGX_ATTR_DEBUG from 0x01 to 0x02. v2: * get_rand_uint32() changed the value of the pointer instead of value where it is pointing at. * Launch enclave incorrectly used sigstruct attributes-field instead of enclave attributes-field. * Removed unused struct sgx_add_page_req from sgx_ioctl.c * Removed unused sgx_has_sgx2. * Updated arch/x86/include/asm/sgx.h so that it provides stub implementations when sgx in not enabled. * Removed cruft rdmsr-calls from sgx_set_pubkeyhash_msrs(). * return -ENOMEM in sgx_alloc_page() when VA pages consume too much space * removed unused global sgx_nr_pids * moved sgx_encl_release to sgx_encl.c * return -ERESTARTSYS instead of -EINTR in sgx_encl_init() Haim Cohen (1): x86: add SGX MSRs to msr-index.h Jarkko Sakkinen (8): intel_sgx: updated MAINTAINERS x86: define IA32_FEATUE_CONTROL.SGX_LC intel_sgx: driver for Intel Software Guard Extensions intel_sgx: ptrace() support intel_sgx: in-kernel launch enclave fs/pipe.c: export create_pipe_files() and replace_fd() intel_sgx: glue code for in-kernel LE intel_sgx: driver documentation Kai Huang (1): x86: add SGX definition to cpufeature Sean Christopherson (1): x86: define IA32_FEATURE_CONTROL.SGX_ENABLE Documentation/index.rst| 1 + Documentation/x86/intel_sgx.rst| 101 +++ MAINTAINERS| 5 + arch/x86/include/asm/cpufeatures.h | 2 + arch/x86/include/asm/msr-index.h | 8 + arch/x86/include/asm/sgx.h | 233 + arch/x86/include/asm/sgx_arch.h| 268 ++ arch/x86/include/uapi/asm/sgx.h| 138 +++ drivers/platform/x86/Kconfig | 2 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_sgx/Kconfig | 34 + drivers/platform/x86/intel_sgx/Makefile| 32 + drivers/platform/x86/intel_sgx/le/Makefile | 26 + drivers/platform/x86/intel_sgx/le/enclave/Makefile | 46 + .../x86/intel_sgx/le/enclave/aes_encrypt.c | 191 .../platform/x86/intel_sgx/le/enclave/cmac_mode.c | 254 ++ .../x86/intel_sgx/le/enclave/encl_bootstrap.S | 163 .../intel_sgx/le/enclave/include/tinycrypt/aes.h | 133 +++ .../le/enclave/include/tinycrypt/cmac_mode.h | 194 .../le/enclave/include/tinycrypt/constants.h | 59 ++ .../intel_sgx/le/enclave/include/tinycrypt/utils.h | 95 ++ drivers/platform/x86/intel_sgx/le/enclave/main.c | 203 + .../platform/x86/intel_sgx/le/enclave/sgx_le.lds | 28 + .../platform/x86/intel_sgx/le/enclave/sgxsign.c| 538 +++ drivers/platform/x86/intel_sgx/le/enclave/utils.c | 78 ++ drivers/platform/x86/intel_sgx/le/entry.S | 117 +++ .../platform/x86/intel_sgx/le/include/sgx_asm.h| 64 ++ .../platform/x86/intel_sgx/le/include/sgx_encl.h | 110 +++ drivers/platform/x86/intel_sgx/le/main.c | 214 + drivers/platform/x86/intel_sgx/le/sgx_le_piggy.S | 15 + drivers/platform/x86/intel_sgx/sgx.h | 268 ++ drivers/platform/x86/intel_sgx/sgx_encl.c | 999 + drivers/platform/x86/intel_sgx/sgx_ioctl.c | 282 ++ drivers/platform/x86/intel_sgx/sgx_le.c| 319 +++ .../platform/x86/intel_sgx/sgx_le_proxy_piggy.S| 15 + drivers/platform/x86/intel_sgx/sgx_main.c | 456 ++ drivers/platform/x86/intel_sgx/sgx_page_cache.c| 619 + drivers/platform/x86/intel_sgx/sgx_util.c | 394 drivers/platform/x86/intel_sgx/sgx_vma.c | 236 + fs/file.c | 1 + fs/pipe.c | 1 + 41 files changed, 6943 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst create mode 100644 arch/x86/include/asm/sgx.h create mode 100644 arch/x86/include/asm/sgx_arch.h create mode 100644 arch/x86/include/uapi/asm/sgx.h create mode 100644 drivers/platform/x86/intel_sgx/Kconfig create mode 100644 drivers/platform/x86/intel_sgx/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/aes_encrypt.c create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/cmac_mode.c create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/encl_bootstrap.S create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/include/tinycrypt/aes.h create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/include/tinycrypt/cmac_mode.h create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/include/tinycrypt/constants.h create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/
[PATCH v6 11/11] intel_sgx: driver documentation
Signed-off-by: Jarkko Sakkinen --- Documentation/index.rst | 1 + Documentation/x86/intel_sgx.rst | 101 2 files changed, 102 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst diff --git a/Documentation/index.rst b/Documentation/index.rst index cb7f1ba5b3b1..ccfebc260e04 100644 --- a/Documentation/index.rst +++ b/Documentation/index.rst @@ -86,6 +86,7 @@ implementation. :maxdepth: 2 sh/index + x86/index Korean translations --- diff --git a/Documentation/x86/intel_sgx.rst b/Documentation/x86/intel_sgx.rst new file mode 100644 index ..59049a35512f --- /dev/null +++ b/Documentation/x86/intel_sgx.rst @@ -0,0 +1,101 @@ +=== +Intel(R) SGX driver +=== + +Introduction + + +Intel(R) SGX is a set of CPU instructions that can be used by applications to +set aside private regions of code and data. The code outside the enclave is +disallowed to access the memory inside the enclave by the CPU access control. +In a way you can think that SGX provides inverted sandbox. It protects the +application from a malicious host. + +There is a new hardware unit in the processor called Memory Encryption Engine +(MEE) starting from the Skylake microarchitecture. BIOS can define one or many +MEE regions that can hold enclave data by configuring them with PRMRR registers. + +The MEE automatically encrypts the data leaving the processor package to the MEE +regions. The data is encrypted using a random key whose life-time is exactly one +power cycle. + +You can tell if your CPU supports SGX by looking into ``/proc/cpuinfo``: + + ``cat /proc/cpuinfo | grep sgx`` + +Enclave data types +== + +SGX defines new data types to maintain information about the enclaves and their +security properties. + +The following data structures exist in MEE regions: + +* **Enclave Page Cache (EPC):** memory pages for protected code and data +* **Enclave Page Cache Map (EPCM):** meta-data for each EPC page + +The Enclave Page Cache holds following types of pages: + +* **SGX Enclave Control Structure (SECS)**: meta-data defining the global + properties of an enclave such as range of addresses it can access. +* **Regular (REG):** containing code and data for the enclave. +* **Thread Control Structure (TCS):** defines an entry point for a hardware + thread to enter into the enclave. The enclave can only be entered through + these entry points. +* **Version Array (VA)**: an EPC page receives a unique 8 byte version number + when it is swapped, which is then stored into a VA page. A VA page can hold up + to 512 version numbers. + +Launch control +== + +For launching an enclave, two structures must be provided for ENCLS(EINIT): + +1. **SIGSTRUCT:** a signed measurement of the enclave binary. +2. **EINITTOKEN:** the measurement, the public key of the signer and various + enclave attributes. This structure contains a MAC of its contents using + hardware derived symmetric key called *launch key*. + +The hardware platform contains a root key pair for signing the SIGTRUCT +for a *launch enclave* that is able to acquire the *launch key* for +creating EINITTOKEN's for other enclaves. For the launch enclave +EINITTOKEN is not needed because it is signed with the private root key. + +There are two feature control bits associate with launch control: + +* **IA32_FEATURE_CONTROL[0]**: locks down the feature control register +* **IA32_FEATURE_CONTROL[17]**: allow runtime reconfiguration of + IA32_SGXLEPUBKEYHASHn MSRs that define MRSIGNER hash for the launch + enclave. Essentially they define a signing key that does not require + EINITTOKEN to be let run. + +The BIOS can configure IA32_SGXLEPUBKEYHASHn MSRs before feature control +register is locked. + +It could be tempting to implement launch control by writing the MSRs +every time when an enclave is launched. This does not scale because for +generic case because BIOS might lock down the MSRs before handover to +the OS. + +Debug enclaves +-- + +Enclave can be set as a *debug enclave* of which memory can be read or written +by using the ENCLS(EDBGRD) and ENCLS(EDBGWR) opcodes. The Intel provided launch +enclave provides them always a valid EINITTOKEN and therefore they are a low +hanging fruit way to try out SGX. + +SGX uapi + + +.. kernel-doc:: drivers/platform/x86/intel_sgx_ioctl.c + :functions: sgx_ioc_enclave_create + sgx_ioc_enclave_add_page + sgx_ioc_enclave_init + +.. kernel-doc:: arch/x86/include/uapi/asm/sgx.h + +References +== + +* System Programming Manual: 39.1.4 Intel?? SGX Launch Control Configuration -- 2.14.1 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 11/11] intel_sgx: driver documentation
On Mon, Nov 27, 2017 at 09:03:39AM -0800, Sean Christopherson wrote: > I have a branch based on Jarkko's patches (I believe it's up-to-date with v5) > that implements what I described. I'd be happy to send RFC patches if that > would help. That would only slow things down. The code is easy to move around and I'm doing infrastructure changes as part of the review process. The latest version (v6) that I sent on Sat has struct sgx_epc_page removed just to name an example. Rather than sending any deprecated patches it would be more useful to get input (in English) on directory layout. I guess you missed v6 as it I had to drop the 01.org list temporarily. It will be back in v7 as I was able to retrieve the admin password and configure it in suitable way. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 11/11] intel_sgx: driver documentation
On Tue, Nov 28, 2017 at 10:37:48PM +0200, Jarkko Sakkinen wrote: > On Mon, Nov 27, 2017 at 09:03:39AM -0800, Sean Christopherson wrote: > > I have a branch based on Jarkko's patches (I believe it's up-to-date with > > v5) > > that implements what I described. I'd be happy to send RFC patches if that > > would help. > > That would only slow things down. The code is easy to move around and > I'm doing infrastructure changes as part of the review process. The > latest version (v6) that I sent on Sat has struct sgx_epc_page removed > just to name an example. > > Rather than sending any deprecated patches it would be more useful to > get input (in English) on directory layout. > > I guess you missed v6 as it I had to drop the 01.org list temporarily. > It will be back in v7 as I was able to retrieve the admin password and > configure it in suitable way. Once my series has been landed, you could drop a series for review. Then it does make sense. I won't of course add any exports needed by KVM etc. I'm only doing "lowest common denominator" groundwork. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 0/8] Intel SGX Driver
d missing EEXTEND operations to LE signing and launch. * Fixed SSA size for GPRS region from 168 to 184 bytes. * Fixed the checks for TCS flags. Now DBGOPTIN is allowed. * Check that TCS addresses are in ELRANGE and not just page aligned. * Require kernel to be compiled with X64_64 and CPU_SUP_INTEL. * Fixed an incorrect value for SGX_ATTR_DEBUG from 0x01 to 0x02. v2: * get_rand_uint32() changed the value of the pointer instead of value where it is pointing at. * Launch enclave incorrectly used sigstruct attributes-field instead of enclave attributes-field. * Removed unused struct sgx_add_page_req from sgx_ioctl.c * Removed unused sgx_has_sgx2. * Updated arch/x86/include/asm/sgx.h so that it provides stub implementations when sgx in not enabled. * Removed cruft rdmsr-calls from sgx_set_pubkeyhash_msrs(). * return -ENOMEM in sgx_alloc_page() when VA pages consume too much space * removed unused global sgx_nr_pids * moved sgx_encl_release to sgx_encl.c * return -ERESTARTSYS instead of -EINTR in sgx_encl_init() Jarkko Sakkinen (6): intel_sgx: updated MAINTAINERS intel_sgx: driver for Intel Software Guard Extensions intel_sgx: ptrace() support intel_sgx: driver documentation fs/pipe.c: export create_pipe_files() intel_sgx: in-kernel launch enclave Kai Huang (1): x86: add SGX definitions to cpufeature Sean Christopherson (1): x86: add SGX definitions to msr-index.h Documentation/index.rst| 1 + Documentation/x86/intel_sgx.rst| 101 +++ MAINTAINERS| 7 + arch/x86/include/asm/cpufeatures.h | 2 + arch/x86/include/asm/msr-index.h | 8 + arch/x86/include/asm/sgx.h | 241 + arch/x86/include/asm/sgx_arch.h| 270 ++ arch/x86/include/uapi/asm/sgx.h| 138 +++ drivers/platform/x86/Kconfig | 2 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_sgx/Kconfig | 34 + drivers/platform/x86/intel_sgx/Makefile| 32 + drivers/platform/x86/intel_sgx/le/Makefile | 27 + drivers/platform/x86/intel_sgx/le/enclave/Makefile | 46 + .../x86/intel_sgx/le/enclave/aesni-intel_asm.S | 1 + .../platform/x86/intel_sgx/le/enclave/cmac_mode.c | 258 ++ .../platform/x86/intel_sgx/le/enclave/cmac_mode.h | 103 +++ .../x86/intel_sgx/le/enclave/encl_bootstrap.S | 163 drivers/platform/x86/intel_sgx/le/enclave/main.c | 193 drivers/platform/x86/intel_sgx/le/enclave/main.h | 66 ++ .../platform/x86/intel_sgx/le/enclave/sgx_le.lds | 28 + .../platform/x86/intel_sgx/le/enclave/sgxsign.c| 551 drivers/platform/x86/intel_sgx/le/enclave/string.c | 1 + drivers/platform/x86/intel_sgx/le/entry.S | 117 +++ .../platform/x86/intel_sgx/le/include/sgx_asm.h| 64 ++ drivers/platform/x86/intel_sgx/le/main.c | 205 + drivers/platform/x86/intel_sgx/le/main.h | 81 ++ drivers/platform/x86/intel_sgx/le/sgx_le_piggy.S | 15 + drivers/platform/x86/intel_sgx/le/string.c | 77 ++ drivers/platform/x86/intel_sgx/sgx.h | 275 ++ drivers/platform/x86/intel_sgx/sgx_encl.c | 992 + drivers/platform/x86/intel_sgx/sgx_ioctl.c | 283 ++ drivers/platform/x86/intel_sgx/sgx_le.c| 312 +++ .../platform/x86/intel_sgx/sgx_le_proxy_piggy.S| 15 + drivers/platform/x86/intel_sgx/sgx_main.c | 456 ++ drivers/platform/x86/intel_sgx/sgx_page_cache.c| 647 ++ drivers/platform/x86/intel_sgx/sgx_util.c | 371 drivers/platform/x86/intel_sgx/sgx_vma.c | 236 + fs/pipe.c | 1 + 39 files changed, 6421 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst create mode 100644 arch/x86/include/asm/sgx.h create mode 100644 arch/x86/include/asm/sgx_arch.h create mode 100644 arch/x86/include/uapi/asm/sgx.h create mode 100644 drivers/platform/x86/intel_sgx/Kconfig create mode 100644 drivers/platform/x86/intel_sgx/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/Makefile create mode 12 drivers/platform/x86/intel_sgx/le/enclave/aesni-intel_asm.S create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/cmac_mode.c create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/cmac_mode.h create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/encl_bootstrap.S create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/main.c create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/main.h create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/sgx_le.lds create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/sgxsign.c create mode 12 drivers/platform/x86/inte
[PATCH v7 6/8] intel_sgx: driver documentation
Signed-off-by: Jarkko Sakkinen --- Documentation/index.rst | 1 + Documentation/x86/intel_sgx.rst | 101 2 files changed, 102 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst diff --git a/Documentation/index.rst b/Documentation/index.rst index cb7f1ba5b3b1..ccfebc260e04 100644 --- a/Documentation/index.rst +++ b/Documentation/index.rst @@ -86,6 +86,7 @@ implementation. :maxdepth: 2 sh/index + x86/index Korean translations --- diff --git a/Documentation/x86/intel_sgx.rst b/Documentation/x86/intel_sgx.rst new file mode 100644 index ..59049a35512f --- /dev/null +++ b/Documentation/x86/intel_sgx.rst @@ -0,0 +1,101 @@ +=== +Intel(R) SGX driver +=== + +Introduction + + +Intel(R) SGX is a set of CPU instructions that can be used by applications to +set aside private regions of code and data. The code outside the enclave is +disallowed to access the memory inside the enclave by the CPU access control. +In a way you can think that SGX provides inverted sandbox. It protects the +application from a malicious host. + +There is a new hardware unit in the processor called Memory Encryption Engine +(MEE) starting from the Skylake microarchitecture. BIOS can define one or many +MEE regions that can hold enclave data by configuring them with PRMRR registers. + +The MEE automatically encrypts the data leaving the processor package to the MEE +regions. The data is encrypted using a random key whose life-time is exactly one +power cycle. + +You can tell if your CPU supports SGX by looking into ``/proc/cpuinfo``: + + ``cat /proc/cpuinfo | grep sgx`` + +Enclave data types +== + +SGX defines new data types to maintain information about the enclaves and their +security properties. + +The following data structures exist in MEE regions: + +* **Enclave Page Cache (EPC):** memory pages for protected code and data +* **Enclave Page Cache Map (EPCM):** meta-data for each EPC page + +The Enclave Page Cache holds following types of pages: + +* **SGX Enclave Control Structure (SECS)**: meta-data defining the global + properties of an enclave such as range of addresses it can access. +* **Regular (REG):** containing code and data for the enclave. +* **Thread Control Structure (TCS):** defines an entry point for a hardware + thread to enter into the enclave. The enclave can only be entered through + these entry points. +* **Version Array (VA)**: an EPC page receives a unique 8 byte version number + when it is swapped, which is then stored into a VA page. A VA page can hold up + to 512 version numbers. + +Launch control +== + +For launching an enclave, two structures must be provided for ENCLS(EINIT): + +1. **SIGSTRUCT:** a signed measurement of the enclave binary. +2. **EINITTOKEN:** the measurement, the public key of the signer and various + enclave attributes. This structure contains a MAC of its contents using + hardware derived symmetric key called *launch key*. + +The hardware platform contains a root key pair for signing the SIGTRUCT +for a *launch enclave* that is able to acquire the *launch key* for +creating EINITTOKEN's for other enclaves. For the launch enclave +EINITTOKEN is not needed because it is signed with the private root key. + +There are two feature control bits associate with launch control: + +* **IA32_FEATURE_CONTROL[0]**: locks down the feature control register +* **IA32_FEATURE_CONTROL[17]**: allow runtime reconfiguration of + IA32_SGXLEPUBKEYHASHn MSRs that define MRSIGNER hash for the launch + enclave. Essentially they define a signing key that does not require + EINITTOKEN to be let run. + +The BIOS can configure IA32_SGXLEPUBKEYHASHn MSRs before feature control +register is locked. + +It could be tempting to implement launch control by writing the MSRs +every time when an enclave is launched. This does not scale because for +generic case because BIOS might lock down the MSRs before handover to +the OS. + +Debug enclaves +-- + +Enclave can be set as a *debug enclave* of which memory can be read or written +by using the ENCLS(EDBGRD) and ENCLS(EDBGWR) opcodes. The Intel provided launch +enclave provides them always a valid EINITTOKEN and therefore they are a low +hanging fruit way to try out SGX. + +SGX uapi + + +.. kernel-doc:: drivers/platform/x86/intel_sgx_ioctl.c + :functions: sgx_ioc_enclave_create + sgx_ioc_enclave_add_page + sgx_ioc_enclave_init + +.. kernel-doc:: arch/x86/include/uapi/asm/sgx.h + +References +== + +* System Programming Manual: 39.1.4 Intel?? SGX Launch Control Configuration -- 2.14.1 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 00/11] Intel SGX Driver
On Tue, Dec 12, 2017 at 03:07:50PM +0100, Pavel Machek wrote: > On Sat 2017-11-25 21:29:17, Jarkko Sakkinen wrote: > > Intel(R) SGX is a set of CPU instructions that can be used by applications > > to > > set aside private regions of code and data. The code outside the enclave is > > disallowed to access the memory inside the enclave by the CPU access > > control. > > In a way you can think that SGX provides inverted sandbox. It protects the > > application from a malicious host. > > Would you list guarantees provided by SGX? > > For example, host can still observe timing of cachelines being > accessed by "protected" app, right? Can it also introduce bit flips? > > Pavel I'll put this in my backlog. Thank you. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 0/8] Intel SGX Driver
* Implemented consistent exception handling to __encls() and __encls_ret(). * Implemented a proper device model in order to allow sysfs attributes and in-kernel API. * Cleaned up various "find enclave" implementations to the unified sgx_encl_find(). * Validate that vm_pgoff is zero. * Discard backing pages with shmem_truncate_range() after EADD. * Added missing EEXTEND operations to LE signing and launch. * Fixed SSA size for GPRS region from 168 to 184 bytes. * Fixed the checks for TCS flags. Now DBGOPTIN is allowed. * Check that TCS addresses are in ELRANGE and not just page aligned. * Require kernel to be compiled with X64_64 and CPU_SUP_INTEL. * Fixed an incorrect value for SGX_ATTR_DEBUG from 0x01 to 0x02. v2: * get_rand_uint32() changed the value of the pointer instead of value where it is pointing at. * Launch enclave incorrectly used sigstruct attributes-field instead of enclave attributes-field. * Removed unused struct sgx_add_page_req from sgx_ioctl.c * Removed unused sgx_has_sgx2. * Updated arch/x86/include/asm/sgx.h so that it provides stub implementations when sgx in not enabled. * Removed cruft rdmsr-calls from sgx_set_pubkeyhash_msrs(). * return -ENOMEM in sgx_alloc_page() when VA pages consume too much space * removed unused global sgx_nr_pids * moved sgx_encl_release to sgx_encl.c * return -ERESTARTSYS instead of -EINTR in sgx_encl_init() Jarkko Sakkinen (6): intel_sgx: updated MAINTAINERS intel_sgx: driver for Intel Software Guard Extensions intel_sgx: ptrace() support intel_sgx: driver documentation fs/pipe.c: export create_pipe_files() intel_sgx: in-kernel launch enclave Kai Huang (1): x86: add SGX definitions to cpufeature Sean Christopherson (1): x86: add SGX definitions to msr-index.h Documentation/index.rst| 1 + Documentation/x86/intel_sgx.rst| 101 +++ MAINTAINERS| 7 + arch/x86/include/asm/cpufeatures.h | 2 + arch/x86/include/asm/msr-index.h | 8 + arch/x86/include/asm/sgx.h | 245 + arch/x86/include/asm/sgx_arch.h| 270 ++ arch/x86/include/uapi/asm/sgx.h| 138 +++ drivers/platform/x86/Kconfig | 2 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_sgx/Kconfig | 34 + drivers/platform/x86/intel_sgx/Makefile| 32 + drivers/platform/x86/intel_sgx/le/Makefile | 27 + drivers/platform/x86/intel_sgx/le/enclave/Makefile | 46 + .../x86/intel_sgx/le/enclave/aesni-intel_asm.S | 1 + .../platform/x86/intel_sgx/le/enclave/cmac_mode.c | 258 ++ .../platform/x86/intel_sgx/le/enclave/cmac_mode.h | 103 +++ .../x86/intel_sgx/le/enclave/encl_bootstrap.S | 163 drivers/platform/x86/intel_sgx/le/enclave/main.c | 193 drivers/platform/x86/intel_sgx/le/enclave/main.h | 66 ++ .../platform/x86/intel_sgx/le/enclave/sgx_le.lds | 28 + .../platform/x86/intel_sgx/le/enclave/sgxsign.c| 551 drivers/platform/x86/intel_sgx/le/enclave/string.c | 1 + drivers/platform/x86/intel_sgx/le/entry.S | 117 +++ .../platform/x86/intel_sgx/le/include/sgx_asm.h| 64 ++ drivers/platform/x86/intel_sgx/le/main.c | 186 drivers/platform/x86/intel_sgx/le/main.h | 78 ++ drivers/platform/x86/intel_sgx/le/sgx_le_piggy.S | 15 + drivers/platform/x86/intel_sgx/le/string.c | 77 ++ drivers/platform/x86/intel_sgx/sgx.h | 283 ++ drivers/platform/x86/intel_sgx/sgx_encl.c | 992 + drivers/platform/x86/intel_sgx/sgx_ioctl.c | 283 ++ drivers/platform/x86/intel_sgx/sgx_le.c| 307 +++ .../platform/x86/intel_sgx/sgx_le_proxy_piggy.S| 15 + drivers/platform/x86/intel_sgx/sgx_main.c | 483 ++ drivers/platform/x86/intel_sgx/sgx_page_cache.c| 642 + drivers/platform/x86/intel_sgx/sgx_util.c | 372 drivers/platform/x86/intel_sgx/sgx_vma.c | 236 + fs/pipe.c | 1 + 39 files changed, 6429 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst create mode 100644 arch/x86/include/asm/sgx.h create mode 100644 arch/x86/include/asm/sgx_arch.h create mode 100644 arch/x86/include/uapi/asm/sgx.h create mode 100644 drivers/platform/x86/intel_sgx/Kconfig create mode 100644 drivers/platform/x86/intel_sgx/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/Makefile create mode 12 drivers/platform/x86/intel_sgx/le/enclave/aesni-intel_asm.S create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/cmac_mode.c create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/cmac_mode.h create mode 100644 drivers/platform/x86/inte
[PATCH v8 6/8] intel_sgx: driver documentation
Signed-off-by: Jarkko Sakkinen Tested-by: Serge Ayoun --- Documentation/index.rst | 1 + Documentation/x86/intel_sgx.rst | 101 2 files changed, 102 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst diff --git a/Documentation/index.rst b/Documentation/index.rst index cb7f1ba5b3b1..ccfebc260e04 100644 --- a/Documentation/index.rst +++ b/Documentation/index.rst @@ -86,6 +86,7 @@ implementation. :maxdepth: 2 sh/index + x86/index Korean translations --- diff --git a/Documentation/x86/intel_sgx.rst b/Documentation/x86/intel_sgx.rst new file mode 100644 index ..59049a35512f --- /dev/null +++ b/Documentation/x86/intel_sgx.rst @@ -0,0 +1,101 @@ +=== +Intel(R) SGX driver +=== + +Introduction + + +Intel(R) SGX is a set of CPU instructions that can be used by applications to +set aside private regions of code and data. The code outside the enclave is +disallowed to access the memory inside the enclave by the CPU access control. +In a way you can think that SGX provides inverted sandbox. It protects the +application from a malicious host. + +There is a new hardware unit in the processor called Memory Encryption Engine +(MEE) starting from the Skylake microarchitecture. BIOS can define one or many +MEE regions that can hold enclave data by configuring them with PRMRR registers. + +The MEE automatically encrypts the data leaving the processor package to the MEE +regions. The data is encrypted using a random key whose life-time is exactly one +power cycle. + +You can tell if your CPU supports SGX by looking into ``/proc/cpuinfo``: + + ``cat /proc/cpuinfo | grep sgx`` + +Enclave data types +== + +SGX defines new data types to maintain information about the enclaves and their +security properties. + +The following data structures exist in MEE regions: + +* **Enclave Page Cache (EPC):** memory pages for protected code and data +* **Enclave Page Cache Map (EPCM):** meta-data for each EPC page + +The Enclave Page Cache holds following types of pages: + +* **SGX Enclave Control Structure (SECS)**: meta-data defining the global + properties of an enclave such as range of addresses it can access. +* **Regular (REG):** containing code and data for the enclave. +* **Thread Control Structure (TCS):** defines an entry point for a hardware + thread to enter into the enclave. The enclave can only be entered through + these entry points. +* **Version Array (VA)**: an EPC page receives a unique 8 byte version number + when it is swapped, which is then stored into a VA page. A VA page can hold up + to 512 version numbers. + +Launch control +== + +For launching an enclave, two structures must be provided for ENCLS(EINIT): + +1. **SIGSTRUCT:** a signed measurement of the enclave binary. +2. **EINITTOKEN:** the measurement, the public key of the signer and various + enclave attributes. This structure contains a MAC of its contents using + hardware derived symmetric key called *launch key*. + +The hardware platform contains a root key pair for signing the SIGTRUCT +for a *launch enclave* that is able to acquire the *launch key* for +creating EINITTOKEN's for other enclaves. For the launch enclave +EINITTOKEN is not needed because it is signed with the private root key. + +There are two feature control bits associate with launch control: + +* **IA32_FEATURE_CONTROL[0]**: locks down the feature control register +* **IA32_FEATURE_CONTROL[17]**: allow runtime reconfiguration of + IA32_SGXLEPUBKEYHASHn MSRs that define MRSIGNER hash for the launch + enclave. Essentially they define a signing key that does not require + EINITTOKEN to be let run. + +The BIOS can configure IA32_SGXLEPUBKEYHASHn MSRs before feature control +register is locked. + +It could be tempting to implement launch control by writing the MSRs +every time when an enclave is launched. This does not scale because for +generic case because BIOS might lock down the MSRs before handover to +the OS. + +Debug enclaves +-- + +Enclave can be set as a *debug enclave* of which memory can be read or written +by using the ENCLS(EDBGRD) and ENCLS(EDBGWR) opcodes. The Intel provided launch +enclave provides them always a valid EINITTOKEN and therefore they are a low +hanging fruit way to try out SGX. + +SGX uapi + + +.. kernel-doc:: drivers/platform/x86/intel_sgx_ioctl.c + :functions: sgx_ioc_enclave_create + sgx_ioc_enclave_add_page + sgx_ioc_enclave_init + +.. kernel-doc:: arch/x86/include/uapi/asm/sgx.h + +References +== + +* System Programming Manual: 39.1.4 Intel?? SGX Launch Control Configuration -- 2.14.1 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 0/7] Intel SGX Driver
is zero. * Discard backing pages with shmem_truncate_range() after EADD. * Added missing EEXTEND operations to LE signing and launch. * Fixed SSA size for GPRS region from 168 to 184 bytes. * Fixed the checks for TCS flags. Now DBGOPTIN is allowed. * Check that TCS addresses are in ELRANGE and not just page aligned. * Require kernel to be compiled with X64_64 and CPU_SUP_INTEL. * Fixed an incorrect value for SGX_ATTR_DEBUG from 0x01 to 0x02. v2: * get_rand_uint32() changed the value of the pointer instead of value where it is pointing at. * Launch enclave incorrectly used sigstruct attributes-field instead of enclave attributes-field. * Removed unused struct sgx_add_page_req from sgx_ioctl.c * Removed unused sgx_has_sgx2. * Updated arch/x86/include/asm/sgx.h so that it provides stub implementations when sgx in not enabled. * Removed cruft rdmsr-calls from sgx_set_pubkeyhash_msrs(). * return -ENOMEM in sgx_alloc_page() when VA pages consume too much space * removed unused global sgx_nr_pids * moved sgx_encl_release to sgx_encl.c * return -ERESTARTSYS instead of -EINTR in sgx_encl_init() Jarkko Sakkinen (5): intel_sgx: updated MAINTAINERS intel_sgx: driver for Intel Software Guard Extensions intel_sgx: ptrace() support intel_sgx: driver documentation intel_sgx: in-kernel launch enclave Kai Huang (1): x86: add SGX definitions to cpufeature Sean Christopherson (1): x86: add SGX definitions to msr-index.h Documentation/index.rst| 1 + Documentation/x86/intel_sgx.rst| 101 +++ MAINTAINERS| 7 + arch/x86/include/asm/cpufeatures.h | 2 + arch/x86/include/asm/msr-index.h | 8 + arch/x86/include/asm/sgx.h | 241 + arch/x86/include/asm/sgx_arch.h| 270 ++ arch/x86/include/asm/sgx_le.h | 67 ++ arch/x86/include/uapi/asm/sgx.h| 138 +++ drivers/platform/x86/Kconfig | 2 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_sgx/Kconfig | 34 + drivers/platform/x86/intel_sgx/Makefile| 32 + drivers/platform/x86/intel_sgx/le/Makefile | 27 + drivers/platform/x86/intel_sgx/le/enclave/Makefile | 46 + .../x86/intel_sgx/le/enclave/aesni-intel_asm.S | 1 + .../platform/x86/intel_sgx/le/enclave/cmac_mode.c | 258 ++ .../platform/x86/intel_sgx/le/enclave/cmac_mode.h | 103 +++ .../x86/intel_sgx/le/enclave/encl_bootstrap.S | 163 drivers/platform/x86/intel_sgx/le/enclave/main.c | 193 drivers/platform/x86/intel_sgx/le/enclave/main.h | 66 ++ .../platform/x86/intel_sgx/le/enclave/sgx_le.lds | 28 + .../platform/x86/intel_sgx/le/enclave/sgxsign.c| 551 drivers/platform/x86/intel_sgx/le/enclave/string.c | 1 + drivers/platform/x86/intel_sgx/le/entry.S | 118 +++ .../platform/x86/intel_sgx/le/include/sgx_asm.h| 64 ++ drivers/platform/x86/intel_sgx/le/main.c | 187 drivers/platform/x86/intel_sgx/le/main.h | 78 ++ drivers/platform/x86/intel_sgx/le/sgx_le_piggy.S | 15 + drivers/platform/x86/intel_sgx/le/string.c | 77 ++ drivers/platform/x86/intel_sgx/sgx.h | 283 ++ drivers/platform/x86/intel_sgx/sgx_encl.c | 992 + drivers/platform/x86/intel_sgx/sgx_ioctl.c | 283 ++ drivers/platform/x86/intel_sgx/sgx_le.c| 317 +++ .../platform/x86/intel_sgx/sgx_le_proxy_piggy.S| 15 + drivers/platform/x86/intel_sgx/sgx_main.c | 483 ++ drivers/platform/x86/intel_sgx/sgx_page_cache.c| 642 + drivers/platform/x86/intel_sgx/sgx_util.c | 372 drivers/platform/x86/intel_sgx/sgx_vma.c | 236 + 39 files changed, 6503 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst create mode 100644 arch/x86/include/asm/sgx.h create mode 100644 arch/x86/include/asm/sgx_arch.h create mode 100644 arch/x86/include/asm/sgx_le.h create mode 100644 arch/x86/include/uapi/asm/sgx.h create mode 100644 drivers/platform/x86/intel_sgx/Kconfig create mode 100644 drivers/platform/x86/intel_sgx/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/Makefile create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/Makefile create mode 12 drivers/platform/x86/intel_sgx/le/enclave/aesni-intel_asm.S create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/cmac_mode.c create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/cmac_mode.h create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/encl_bootstrap.S create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/main.c create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/main.h create mode 100644 drivers/platform/x86/intel_sgx/le/enclave/sgx_le.lds create mode 100644 drivers/
[PATCH v9 6/7] intel_sgx: driver documentation
Signed-off-by: Jarkko Sakkinen Tested-by: Serge Ayoun --- Documentation/index.rst | 1 + Documentation/x86/intel_sgx.rst | 101 2 files changed, 102 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst diff --git a/Documentation/index.rst b/Documentation/index.rst index cb7f1ba5b3b1..ccfebc260e04 100644 --- a/Documentation/index.rst +++ b/Documentation/index.rst @@ -86,6 +86,7 @@ implementation. :maxdepth: 2 sh/index + x86/index Korean translations --- diff --git a/Documentation/x86/intel_sgx.rst b/Documentation/x86/intel_sgx.rst new file mode 100644 index ..59049a35512f --- /dev/null +++ b/Documentation/x86/intel_sgx.rst @@ -0,0 +1,101 @@ +=== +Intel(R) SGX driver +=== + +Introduction + + +Intel(R) SGX is a set of CPU instructions that can be used by applications to +set aside private regions of code and data. The code outside the enclave is +disallowed to access the memory inside the enclave by the CPU access control. +In a way you can think that SGX provides inverted sandbox. It protects the +application from a malicious host. + +There is a new hardware unit in the processor called Memory Encryption Engine +(MEE) starting from the Skylake microarchitecture. BIOS can define one or many +MEE regions that can hold enclave data by configuring them with PRMRR registers. + +The MEE automatically encrypts the data leaving the processor package to the MEE +regions. The data is encrypted using a random key whose life-time is exactly one +power cycle. + +You can tell if your CPU supports SGX by looking into ``/proc/cpuinfo``: + + ``cat /proc/cpuinfo | grep sgx`` + +Enclave data types +== + +SGX defines new data types to maintain information about the enclaves and their +security properties. + +The following data structures exist in MEE regions: + +* **Enclave Page Cache (EPC):** memory pages for protected code and data +* **Enclave Page Cache Map (EPCM):** meta-data for each EPC page + +The Enclave Page Cache holds following types of pages: + +* **SGX Enclave Control Structure (SECS)**: meta-data defining the global + properties of an enclave such as range of addresses it can access. +* **Regular (REG):** containing code and data for the enclave. +* **Thread Control Structure (TCS):** defines an entry point for a hardware + thread to enter into the enclave. The enclave can only be entered through + these entry points. +* **Version Array (VA)**: an EPC page receives a unique 8 byte version number + when it is swapped, which is then stored into a VA page. A VA page can hold up + to 512 version numbers. + +Launch control +== + +For launching an enclave, two structures must be provided for ENCLS(EINIT): + +1. **SIGSTRUCT:** a signed measurement of the enclave binary. +2. **EINITTOKEN:** the measurement, the public key of the signer and various + enclave attributes. This structure contains a MAC of its contents using + hardware derived symmetric key called *launch key*. + +The hardware platform contains a root key pair for signing the SIGTRUCT +for a *launch enclave* that is able to acquire the *launch key* for +creating EINITTOKEN's for other enclaves. For the launch enclave +EINITTOKEN is not needed because it is signed with the private root key. + +There are two feature control bits associate with launch control: + +* **IA32_FEATURE_CONTROL[0]**: locks down the feature control register +* **IA32_FEATURE_CONTROL[17]**: allow runtime reconfiguration of + IA32_SGXLEPUBKEYHASHn MSRs that define MRSIGNER hash for the launch + enclave. Essentially they define a signing key that does not require + EINITTOKEN to be let run. + +The BIOS can configure IA32_SGXLEPUBKEYHASHn MSRs before feature control +register is locked. + +It could be tempting to implement launch control by writing the MSRs +every time when an enclave is launched. This does not scale because for +generic case because BIOS might lock down the MSRs before handover to +the OS. + +Debug enclaves +-- + +Enclave can be set as a *debug enclave* of which memory can be read or written +by using the ENCLS(EDBGRD) and ENCLS(EDBGWR) opcodes. The Intel provided launch +enclave provides them always a valid EINITTOKEN and therefore they are a low +hanging fruit way to try out SGX. + +SGX uapi + + +.. kernel-doc:: drivers/platform/x86/intel_sgx_ioctl.c + :functions: sgx_ioc_enclave_create + sgx_ioc_enclave_add_page + sgx_ioc_enclave_init + +.. kernel-doc:: arch/x86/include/uapi/asm/sgx.h + +References +== + +* System Programming Manual: 39.1.4 Intel?? SGX Launch Control Configuration -- 2.14.1 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 00/11] Intel SGX Driver
On Tue, 2017-12-12 at 15:07 +0100, Pavel Machek wrote: > On Sat 2017-11-25 21:29:17, Jarkko Sakkinen wrote: > > Intel(R) SGX is a set of CPU instructions that can be used by applications > > to > > set aside private regions of code and data. The code outside the enclave is > > disallowed to access the memory inside the enclave by the CPU access > > control. > > In a way you can think that SGX provides inverted sandbox. It protects the > > application from a malicious host. > > Would you list guarantees provided by SGX? > > For example, host can still observe timing of cachelines being > accessed by "protected" app, right? Can it also introduce bit flips? > > Pavel I'll give a more proper response to this now that all the reported major issues in the code have been fixed in v9. Yes, SGX is vulnerable to the L1 cacheline timing attacks. Jethro Beekman wrote a great summary about this on early March: https://jbeekman.nl/blog/2017/03/sgx-side-channel-attacks/ The counter measures are the same as without SGX. It really does not add or degrade security in this area. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 00/11] Intel SGX Driver
On Wed, Dec 20, 2017 at 01:33:46AM +0200, Jarkko Sakkinen wrote: > On Tue, 2017-12-12 at 15:07 +0100, Pavel Machek wrote: > > On Sat 2017-11-25 21:29:17, Jarkko Sakkinen wrote: > > > Intel(R) SGX is a set of CPU instructions that can be used by > > > applications to > > > set aside private regions of code and data. The code outside the enclave > > > is > > > disallowed to access the memory inside the enclave by the CPU access > > > control. > > > In a way you can think that SGX provides inverted sandbox. It protects the > > > application from a malicious host. > > > > Would you list guarantees provided by SGX? > > > > For example, host can still observe timing of cachelines being > > accessed by "protected" app, right? Can it also introduce bit flips? > > > > Pavel > > I'll give a more proper response to this now that all the reported major > issues in the code have been fixed in v9. > > Yes, SGX is vulnerable to the L1 cacheline timing attacks. Jethro > Beekman wrote a great summary about this on early March: > > https://jbeekman.nl/blog/2017/03/sgx-side-channel-attacks/ > > The counter measures are the same as without SGX. It really does not > add or degrade security in this area. This came up even in my patch set :-) I.e. I switched to kernel AES-NI from TinyCrypt's AES because the latter is not timing resistant. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 0/7] Intel SGX Driver
egister() with plain bus_register(). * Retry EINIT 2nd time only if MSRs are not locked. v3: * Check that FEATURE_CONTROL_LOCKED and FEATURE_CONTROL_SGX_ENABLE are set. * Return -ERESTARTSYS in __sgx_encl_add_page() when sgx_alloc_page() fails. * Use unused bits in epc_page->pa to store the bank number. * Removed #ifdef for WQ_NONREENTRANT. * If mmu_notifier_register() fails with -EINTR, return -ERESTARTSYS. * Added --remove-section=.got.plt to objcopy flags in order to prevent a dummy .got.plt, which will cause an inconsistent size for the LE. * Documented sgx_encl_* functions. * Added remark about AES implementation used inside the LE. * Removed redundant sgx_sys_exit() from le/main.c. * Fixed struct sgx_secinfo alignment from 128 to 64 bytes. * Validate miscselect in sgx_encl_create(). * Fixed SSA frame size calculation to take the misc region into account. * Implemented consistent exception handling to __encls() and __encls_ret(). * Implemented a proper device model in order to allow sysfs attributes and in-kernel API. * Cleaned up various "find enclave" implementations to the unified sgx_encl_find(). * Validate that vm_pgoff is zero. * Discard backing pages with shmem_truncate_range() after EADD. * Added missing EEXTEND operations to LE signing and launch. * Fixed SSA size for GPRS region from 168 to 184 bytes. * Fixed the checks for TCS flags. Now DBGOPTIN is allowed. * Check that TCS addresses are in ELRANGE and not just page aligned. * Require kernel to be compiled with X64_64 and CPU_SUP_INTEL. * Fixed an incorrect value for SGX_ATTR_DEBUG from 0x01 to 0x02. v2: * get_rand_uint32() changed the value of the pointer instead of value where it is pointing at. * Launch enclave incorrectly used sigstruct attributes-field instead of enclave attributes-field. * Removed unused struct sgx_add_page_req from sgx_ioctl.c * Removed unused sgx_has_sgx2. * Updated arch/x86/include/asm/sgx.h so that it provides stub implementations when sgx in not enabled. * Removed cruft rdmsr-calls from sgx_set_pubkeyhash_msrs(). * return -ENOMEM in sgx_alloc_page() when VA pages consume too much space * removed unused global sgx_nr_pids * moved sgx_encl_release to sgx_encl.c * return -ERESTARTSYS instead of -EINTR in sgx_encl_init() Jarkko Sakkinen (5): intel_sgx: updated MAINTAINERS intel_sgx: driver for Intel Software Guard Extensions intel_sgx: ptrace() support intel_sgx: driver documentation intel_sgx: in-kernel launch enclave Kai Huang (1): x86: add SGX definitions to cpufeature Sean Christopherson (1): x86: add SGX definitions to msr-index.h Documentation/index.rst| 1 + Documentation/x86/intel_sgx.rst| 168 MAINTAINERS| 7 + arch/x86/include/asm/cpufeatures.h | 2 + arch/x86/include/asm/msr-index.h | 8 + arch/x86/include/asm/sgx.h | 192 + arch/x86/include/asm/sgx_arch.h| 222 + arch/x86/include/asm/sgx_le.h | 17 + arch/x86/include/uapi/asm/sgx.h| 138 +++ drivers/platform/x86/Kconfig | 2 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_sgx/Kconfig | 34 + drivers/platform/x86/intel_sgx/Makefile| 32 + drivers/platform/x86/intel_sgx/le/Makefile | 34 + drivers/platform/x86/intel_sgx/le/enclave/Makefile | 53 ++ .../x86/intel_sgx/le/enclave/aesni-intel_asm.S | 1 + .../platform/x86/intel_sgx/le/enclave/cmac_mode.c | 209 + .../platform/x86/intel_sgx/le/enclave/cmac_mode.h | 54 ++ .../x86/intel_sgx/le/enclave/encl_bootstrap.S | 114 +++ drivers/platform/x86/intel_sgx/le/enclave/main.c | 146 drivers/platform/x86/intel_sgx/le/enclave/main.h | 19 + .../platform/x86/intel_sgx/le/enclave/sgx_le.lds | 28 + .../platform/x86/intel_sgx/le/enclave/sgxsign.c| 551 drivers/platform/x86/intel_sgx/le/enclave/string.c | 1 + drivers/platform/x86/intel_sgx/le/entry.S | 69 ++ .../platform/x86/intel_sgx/le/include/sgx_asm.h| 15 + drivers/platform/x86/intel_sgx/le/main.c | 138 +++ drivers/platform/x86/intel_sgx/le/main.h | 29 + drivers/platform/x86/intel_sgx/le/sgx_le_piggy.S | 22 + drivers/platform/x86/intel_sgx/le/string.c | 28 + drivers/platform/x86/intel_sgx/sgx.h | 237 ++ drivers/platform/x86/intel_sgx/sgx_encl.c | 944 + drivers/platform/x86/intel_sgx/sgx_ioctl.c | 234 + drivers/platform/x86/intel_sgx/sgx_le.c| 264 ++ .../platform/x86/intel_sgx/sgx_le_proxy_piggy.S| 22 + drivers/platform/x86/intel_sgx/sgx_main.c | 429 ++ drivers/platform/x86/intel_sgx/sgx_page_cache.c| 593 + drivers/platform/x86/intel_sgx/sgx_util.c | 323 ++
[PATCH v10 6/7] intel_sgx: driver documentation
Signed-off-by: Jarkko Sakkinen Tested-by: Serge Ayoun --- Documentation/index.rst | 1 + Documentation/x86/intel_sgx.rst | 168 2 files changed, 169 insertions(+) create mode 100644 Documentation/x86/intel_sgx.rst diff --git a/Documentation/index.rst b/Documentation/index.rst index cb7f1ba5b3b1..ccfebc260e04 100644 --- a/Documentation/index.rst +++ b/Documentation/index.rst @@ -86,6 +86,7 @@ implementation. :maxdepth: 2 sh/index + x86/index Korean translations --- diff --git a/Documentation/x86/intel_sgx.rst b/Documentation/x86/intel_sgx.rst new file mode 100644 index ..3a49415be62b --- /dev/null +++ b/Documentation/x86/intel_sgx.rst @@ -0,0 +1,168 @@ +=== +Intel(R) SGX driver +=== + +Introduction + + +Intel(R) SGX is a set of CPU instructions that can be used by applications to +set aside private regions of code and data. The code outside the enclave is +disallowed to access the memory inside the enclave by the CPU access control. +In a way you can think that SGX provides inverted sandbox. It protects the +application from a malicious host. + +You can tell if your CPU supports SGX by looking into ``/proc/cpuinfo``: + + ``cat /proc/cpuinfo | grep sgx`` + +Overview of SGX +=== + +SGX has a set of data structures to maintain information about the enclaves and +their security properties. BIOS reserves a fixed size region of physical memory +for these structures by setting Processor Reserved Memory Range Registers +(PRMRR). + +This memory range is protected from outside access by the CPU and all the data +coming in and out of the CPU package is encrypted by a key that is generated for +each boot cycle. + +Enclaves execute in ring-3 in a special enclave submode using pages from the +reserved memory range. A fixed logical address range for the enclave is reserved +by ENCLS(ECREATE), a leaf instruction used to create enclaves. It is referred in +the documentation commonly as the ELRANGE. + +Every memory access to the ELRANGE is asserted by the CPU. If the CPU is not +executing in the enclave mode inside the enclave, #GP is raised. On the other +hand enclave code can make memory accesses both inside and outside of the +ELRANGE. + +Enclave can only execute code inside the ELRANGE. Instructions that may cause +VMEXIT, IO instructions and instructions that require a privilege change are +prohibited inside the enclave. Interrupts and exceptions always cause enclave +to exit and jump to an address outside the enclave given when the enclave is +entered by using the leaf instruction ENCLS(EENTER). + +Data types +-- + +The protected memory range contains the following data: + +* **Enclave Page Cache (EPC):** protected pages +* **Enclave Page Cache Map (EPCM):** a database that describes the state of the + pages and link them to an enclave. + +EPC has a number of different types of pages: + +* **SGX Enclave Control Structure (SECS)**: describes the global + properties of an enclave. +* **Regular (REG):** code and data pages in the ELRANGE. +* **Thread Control Structure (TCS):** pages that define entry points inside an + enclave. The enclave can only be entered through these entry points and each + can host a single hardware thread at a time. +* **Version Array (VA)**: 64-bit version numbers for pages that have been + swapped outside the enclave. Each page contains 512 version numbers. + +Launch control +-- + +To launch an enclave, two structures must be provided for ENCLS(EINIT): + +1. **SIGSTRUCT:** signed measurement of the enclave binary. +2. **EINITTOKEN:** a cryptographic token CMAC-signed with a AES256-key called + *launch key*, which is re-generated for each boot cycle. + +The CPU holds a SHA256 hash of a 3072-bit RSA public key inside +IA32_SGXLEPUBKEYHASHn MSRs. Enclaves with a SIGSTRUCT that is signed with this +key do not require a valid EINITTOKEN and can be authorized with special +privileges. One of those privileges is ability to acquire the launch key with +ENCLS(EGETKEY). + +**IA32_FEATURE_CONTROL[17]** is used by to BIOS configure whether +IA32_SGXLEPUBKEYHASH MSRs are read-only or read-write before locking the +feature control register and handing over control to the operating system. + +Enclave construction + + +The construction is started by filling out the SECS that contains enclave +address range, privileged attributes and measurement of TCS and REG pages (pages +that will be mapped to the address range) among the other things. This structure +is passed out to the ENCLS(ECREATE) together with a physical address of a page +in EPC that will hold the SECS. + +Then pages are added with ENCLS(EADD) and measured with ENCLS(EEXTEND). Finally +enclave is initialized with ENCLS(EINIT). ENCLS(INIT) checks that the SIGSTRUCT +is signed with the contained public key and that the supplied EINITTOKEN
Re: [PATCH v6 00/11] Intel SGX Driver
On Thu, Jan 04, 2018 at 03:06:43AM -0600, Dr. Greg Wettstein wrote: > If we are talking about the issues motivating the KPTI work I don't > have any useful information beyond what is raging through the industry > right now. > > With respect to SGX, the issues giving rise to KPTI are characteristic > of what this technology is designed to address. The technical 'news' > sites, which are even more of an abomination then usual with this > issue, are talking about privileged information such as credentials, > passwords et.al being leaked by this vulnerability. > > Data committed to enclaves are only accessible by the enclave, even > the kernel, by definition, can't access the memory. Given current > events that is an arguably useful behavior. Exactly. You could think adversary using meltdown leak utilizing malware as having same capabilities as peripheral connected to a bus, which we can defend against with SGX. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 00/11] Intel SGX Driver
On Thu, Jan 04, 2018 at 03:17:24PM +0100, Cedric Blancher wrote: > So how does this protect against the MELTDOWN attack (CVE-2017-5754) > and the MELTATOMBOMBA4 worm which uses this exploit? > > Ced Everything going out of L1 gets encrypted. This is done to defend against peripheral like adversaries and should work also against meltdown. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 00/11] Intel SGX Driver
On Tue, Jan 09, 2018 at 03:50:23PM -0600, Dr. Greg Wettstein wrote: > > Everything going out of L1 gets encrypted. This is done to defend > > against peripheral like adversaries and should work also against > > meltdown. > > I don't believe this is an architecturally correct assertion. The > encryption/decryption occurs at the 'bottom' of the cache heirarchy. You are right and I was wrong. It is plain from L1 to LLC, which implies as you correctly described potential cache missing attacks in addition to timing attacks. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 00/11] Intel SGX Driver
On Thu, Feb 08, 2018 at 09:46:53AM +0100, Pavel Machek wrote: > On Tue 2018-01-09 16:27:30, Jarkko Sakkinen wrote: > > On Thu, Jan 04, 2018 at 03:17:24PM +0100, Cedric Blancher wrote: > > > So how does this protect against the MELTDOWN attack (CVE-2017-5754) > > > and the MELTATOMBOMBA4 worm which uses this exploit? > > > > > > Ced > > > > Everything going out of L1 gets encrypted. This is done to defend > > against peripheral like adversaries and should work also against > > meltdown. > > Yeah, but useless against spectre and ability to introduce bit flips > means this is generally useless... You are right. And what I said was simply false. In fact, the encryption is done in LCC. I'm sorry that I didn't response to my response and gave incorrect info. I simply forgot to do this, no other excuses. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v15 23/23] x86/sgx: Driver documentation
On Thu, Nov 08, 2018 at 09:20:40PM +0200, Jarkko Sakkinen wrote: > On Thu, Nov 08, 2018 at 04:39:42PM +0200, Jarkko Sakkinen wrote: > > On Wed, Nov 07, 2018 at 09:09:37AM -0800, Dave Hansen wrote: > > > On 11/7/18 8:30 AM, Jarkko Sakkinen wrote: > > > >> Does this code run when I type "make kselftest"? If not, I think we > > > >> should rectify that. > > > > No, it doesn't. It is just my backup for the non-SDK user space code > > > > that I've made that I will use to fork my user space SGX projects in > > > > the future. > > > > > > > > I can work-out a selftest (and provide a new patch in the series) but > > > > I'm still wondering what the enclave should do. I would suggest that > > > > we start with an enclave that does just EEXIT and nothing else. > > > > > > Yeah, that's a start. But, a good selftest would include things like > > > faults and error conditions. > > > > Great. We can add more entry points to the enclave for different tests > > but I'll start with a bare minimum. And yeah but ever goes into next > > version I'll document the fault handling. > > For the v17 I'll add exactly two test cases: > > 1. EENTER/EEXIT > 2. EENTER/exception > > So that it will easier to evaluate and demonstrate exception handling. > > /Jarkko Here is my test program now: https://github.com/jsakkine-intel/sgx-selftest It is ~1100 lines ATM. Next I'll deploy it to the kernel tree. It has only (1) now but I'll add (2) too when I convert this to a kernel patch (probably by doing sgx_call() with a NULL pointer). /Jarkko
[PATCH v17 22/23] x86/sgx: SGX documentation
Documentation of the features of the Software Guard eXtensions used by the Linux kernel and basic design choices for the core and driver and functionality. Signed-off-by: Jarkko Sakkinen Co-developed-by: Sean Christopherson Signed-off-by: Sean Christopherson --- Documentation/index.rst | 1 + Documentation/x86/index.rst | 8 ++ Documentation/x86/intel_sgx.rst | 233 3 files changed, 242 insertions(+) create mode 100644 Documentation/x86/index.rst create mode 100644 Documentation/x86/intel_sgx.rst diff --git a/Documentation/index.rst b/Documentation/index.rst index c858c2e66e36..63864826dcd6 100644 --- a/Documentation/index.rst +++ b/Documentation/index.rst @@ -101,6 +101,7 @@ implementation. :maxdepth: 2 sh/index + x86/index Filesystem Documentation diff --git a/Documentation/x86/index.rst b/Documentation/x86/index.rst new file mode 100644 index ..11d5b18d9537 --- /dev/null +++ b/Documentation/x86/index.rst @@ -0,0 +1,8 @@ +== +x86 Architecture Guide +== + +.. toctree:: + :maxdepth: 2 + + intel_sgx diff --git a/Documentation/x86/intel_sgx.rst b/Documentation/x86/intel_sgx.rst new file mode 100644 index ..f51b43f9e125 --- /dev/null +++ b/Documentation/x86/intel_sgx.rst @@ -0,0 +1,233 @@ +=== +Intel(R) SGX driver +=== + +Introduction + + +Intel(R) SGX is a set of CPU instructions that can be used by applications to +set aside private regions of code and data. The code outside the enclave is +disallowed to access the memory inside the enclave by the CPU access control. +In a way you can think that SGX provides inverted sandbox. It protects the +application from a malicious host. + +You can tell if your CPU supports SGX by looking into ``/proc/cpuinfo``: + + ``cat /proc/cpuinfo | grep sgx`` + +Overview of SGX +=== + +SGX has a set of data structures to maintain information about the enclaves and +their security properties. BIOS reserves a fixed size region of physical memory +for these structures by setting Processor Reserved Memory Range Registers +(PRMRR). + +This memory range is protected from outside access by the CPU and all the data +coming in and out of the CPU package is encrypted by a key that is generated for +each boot cycle. + +Enclaves execute in ring-3 in a special enclave submode using pages from the +reserved memory range. A fixed logical address range for the enclave is reserved +by ENCLS(ECREATE), a leaf instruction used to create enclaves. It is referred in +the documentation commonly as the ELRANGE. + +Every memory access to the ELRANGE is asserted by the CPU. If the CPU is not +executing in the enclave mode inside the enclave, #GP is raised. On the other +hand, enclave code can make memory accesses both inside and outside of the +ELRANGE. + +Enclave can only execute code inside the ELRANGE. Instructions that may cause +VMEXIT, IO instructions and instructions that require a privilege change are +prohibited inside the enclave. Interrupts and exceptions always cause enclave +to exit and jump to an address outside the enclave given when the enclave is +entered by using the leaf instruction ENCLS(EENTER). + +Protected memory + + +Enclave Page Cache (EPC) +Physical pages used with enclaves that are protected by the CPU from +unauthorized access. + +Enclave Page Cache Map (EPCM) +A database that describes the properties and state of the pages e.g. their +permissions or to which enclave they belong to. + +Memory Encryption Engine (MEE) integrity tree +Autonomously updated integrity tree. The root of the tree located in on-die +SRAM. + +EPC data types +-- + +SGX Enclave Control Structure (SECS) +Describes the global properties of an enclave. Will not be mapped to the +ELRANGE. + +Regular (REG) +These pages contain code and data. + +Thread Control Structure (TCS) +The pages that define the entry points inside an enclave. An enclave can +only be entered through these entry points and each can host a single +hardware thread at a time. + +Version Array (VA) + The pages contain 64-bit version numbers for pages that have been swapped + outside the enclave. Each page has the capacity of 512 version numbers. + +Launch control +-- + +To launch an enclave, two structures must be provided for ENCLS(EINIT): + +1. **SIGSTRUCT:** signed measurement of the enclave binary. +2. **EINITTOKEN:** a cryptographic token CMAC-signed with a AES256-key called + *launch key*, which is re-generated for each boot cycle. + +The CPU holds a SHA256 hash of a 3072-bit RSA public key inside +IA32_SGXLEPUBKEYHASHn MSRs. Enclaves with a SIGSTRUCT that is signed with this +key do not require a valid EINITTOKEN and can be authorized with special +privileges. One of those privileges is ability to acquire the
Re: [PATCH v17 00/23] Intel SGX1 support
For unknown reason this never reached any MLs (used the same command line for git send-email as usual). /Jarkko On Fri, Nov 16, 2018 at 03:38:08AM +0200, Jarkko Sakkinen wrote: > Intel(R) SGX is a set of CPU instructions that can be used by applications > to set aside private regions of code and data. The code outside the enclave > is disallowed to access the memory inside the enclave by the CPU access > control. In a way you can think that SGX provides inverted sandbox. It > protects the application from a malicious host. > > There is a new hardware unit in the processor called Memory Encryption > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > one or many MEE regions that can hold enclave data by configuring them with > PRMRR registers. > > The MEE automatically encrypts the data leaving the processor package to > the MEE regions. The data is encrypted using a random key whose life-time > is exactly one power cycle. > > The current implementation requires that the firmware sets > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > decide what enclaves it wants run. The implementation does not create > any bottlenecks to support read-only MSRs later on. > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > cat /proc/cpuinfo | grep sgx > > v17: > * Add a simple selftest. > * Fix a null pointer dereference to section->pages when its > allocation fails. > * Add Sean's description of the exception handling to the documentation. > > v16: > * Fixed SOB's in the commits that were a bit corrupted in v15. > * Implemented exceptio handling properly to detect_sgx(). > * Use GENMASK() to define SGX_CPUID_SUB_LEAF_TYPE_MASK. > * Updated the documentation to use rst definition lists. > * Added the missing Documentation/x86/index.rst, which has a link to > intel_sgx.rst. Now the SGX and uapi documentation is properly generated > with 'make htmldocs'. > * While enumerating EPC sections, if an undefined section is found, fail > the driver initialization instead of continuing the initialization. > * Issue a warning if there are more than %SGX_MAX_EPC_SECTIONS. > * Remove copyright notice from arch/x86/include/asm/sgx.h. > * Migrated from ioremap_cache() to memremap(). > > v15: > * Split into more digestable size patches. > * Lots of small fixes and clean ups. > * Signal a "plain" SIGSEGV on an EPCM violation. > > v14: > * Change the comment about X86_FEATURE_SGX_LC from “SGX launch > configuration” to “SGX launch control”. > * Move the SGX-related CPU feature flags as part of the Linux defined > virtual leaf 8. > * Add SGX_ prefix to the constants defining the ENCLS leaf functions. > * Use GENMASK*() and BIT*() in sgx_arch.h instead of raw hex numbers. > * Refine the long description for CONFIG_INTEL_SGX_CORE. > * Do not use pr_*_ratelimited() in the driver. The use of the rate limited > versions is legacy cruft from the prototyping phase. > * Detect sleep with SGX_INVALID_EINIT_TOKEN instead of counting power > cycles. > * Manually prefix with “sgx:” in the core SGX code instead of redefining > pr_fmt. > * Report if IA32_SGXLEPUBKEYHASHx MSRs are not writable in the driver > instead of core because it is a driver requirement. > * Change prompt to bool in the entry for CONFIG_INTEL_SGX_CORE because the > default is ‘n’. > * Rename struct sgx_epc_bank as struct sgx_epc_section in order to match > the SDM. > * Allocate struct sgx_epc_page instances one at a time. > * Use “__iomem void *” pointers for the mapped EPC memory consistently. > * Retry once on SGX_INVALID_TOKEN in sgx_einit() instead of counting power > cycles. > * Call enclave swapping operations directly from the driver instead of > calling them .indirectly through struct sgx_epc_page_ops because indirect > calls are not required yet as the patch set does not contain the KVM > support. > * Added special signal SEGV_SGXERR to notify about SGX EPCM violation > errors. > > v13: > * Always use SGX_CPUID constant instead of a hardcoded value. > * Simplified and documented the macros and functions for ENCLS leaves. > * Enable sgx_free_page() to free active enclave pages on demand > in order to allow sgx_invalidate() to delete enclave pages. > It no longer performs EREMOVE if a page is in the process of > being reclaimed. > * Use PM notifier per enclave so that we don't have to traverse > the global list of active EPC pages to find enclaves. > * Removed unused SGX_LE_ROLLBACK constant from uapi/asm/sgx.h > * Always use ioremap() to map EPC banks as we only support 64-bit kernel. > * Invalidate IA32_SGXLEPUBKEYHASH
[PATCH RFC 02/15] Documentation: replace **** with a hug
In order to comply with the CoC, replace with a hug. Signed-off-by: Jarkko Sakkinen --- Documentation/kernel-hacking/locking.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/kernel-hacking/locking.rst b/Documentation/kernel-hacking/locking.rst index 519673df0e82..e16a850261d5 100644 --- a/Documentation/kernel-hacking/locking.rst +++ b/Documentation/kernel-hacking/locking.rst @@ -958,7 +958,7 @@ grabs a read lock, searches a list, fails to find what it wants, drops the read lock, grabs a write lock and inserts the object has a race condition. -If you don't see why, please stay the fuck away from my code. +If you don't see why, please stay the hug away from my code. Racing Timers: A Kernel Pastime --- -- 2.19.1
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 11:56:52AM -0800, Davidlohr Bueso wrote: > On Fri, 30 Nov 2018, Kees Cook wrote: > > > On Fri, Nov 30, 2018 at 11:27 AM Jarkko Sakkinen > > wrote: > > > > > > In order to comply with the CoC, replace with a hug. > > I hope this is some kind of joke. How would anyone get offended by reading > technical comments? This is all beyond me... Well... Not a joke really but more like conversation starter :-) I had 10h flight from Amsterdam to Portland and one of the things that I did was to read the new CoC properly. This a direct quote from the CoC: "Harassment includes the use of abusive, offensive or degrading language, intimidation, stalking, harassing photography or recording, inappropriate physical contact, sexual imagery and unwelcome sexual advances or requests for sexual favors." Doesn't this fall into this category? Your argument is not that great because you could say that from any LKML discussion. If you don't like hugging, please propose something else :-) /Jarkko
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 11:40:17AM -0800, Kees Cook wrote: > On Fri, Nov 30, 2018 at 11:27 AM Jarkko Sakkinen > wrote: > > > > In order to comply with the CoC, replace with a hug. > > Heh. I support the replacement of the stronger language, but I find > "hug", "hugged", and "hugging" to be very weird replacements. Can we > bikeshed this to "heck", "hecked", and "hecking" (or "heckin" to > follow true Doggo meme style). > > "This API is hugged" doesn't make any sense to me. "This API is > hecked" is better, or at least funnier (to me). "Hug this interface" > similarly makes no sense, but "Heck this interface" seems better. > "Don't touch my hecking code", "What the heck were they thinking?" > etc... "hug" is odd. > > Better yet, since it's only 17 files, how about doing context-specific > changes? "This API is terrible", "Hateful interface", "Don't touch my > freakin' code", "What in the world were they thinking?" etc? I'm happy to refine this (thus the RFC tag)! And depending on the culture, hugging could fall in the harrasment category. Actually, when I think about it, in Finland this kind of poking of ones personal bubble would be such :-) I'll refine the patch set with more context sensitive replacements, perhaps removing the comment altogether in some places. Thank you for the feedback! /Jarkko
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 09:09:48PM +0100, John Paul Adrian Glaubitz wrote: > Or just leave it as is because we're all grown up and don't freak out > when a piece of text contains the word "fuck". > > I still don't understand why people think that the word "fuck" is what > would keep certain groups from contributing to the Linux kernel. In all > seriousness, it doesn't. Are you making a claim that your personal experience, and maybe your mates, is the objective truth, or am I misunderstanding something? /Jarkko
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 09:31:13PM +0100, Matthias Brugger wrote: > Like John I don't think that the word "fuck" is something we have to ban from > the source code, but I don't care too much. Anyway, please don't change it to > something like heck as it might be difficult for non-english speaker to > understand. I make context sensitive better thought updates based on the feedback that Kees gave. I used RFC tag for a reason. /Jarkko
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 12:35:07PM -0800, David Miller wrote: > From: Jens Axboe > Date: Fri, 30 Nov 2018 13:12:26 -0700 > > > On 11/30/18 12:56 PM, Davidlohr Bueso wrote: > >> On Fri, 30 Nov 2018, Kees Cook wrote: > >> > >>> On Fri, Nov 30, 2018 at 11:27 AM Jarkko Sakkinen > >>> wrote: > >>>> > >>>> In order to comply with the CoC, replace with a hug. > >> > >> I hope this is some kind of joke. How would anyone get offended by reading > >> technical comments? This is all beyond me... > > > > Agree, this is insanity. > > And even worse it is censorship. It is not close to a cencorship, especially when I used RFC tag, which essentially says that I'm not saying "please take this", right? Can you tell how the CoC should be interpreted then? I read it through on my plane trip with an eye glass. Is cursing OK? /Jarkko
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 01:01:02PM -0800, James Bottomley wrote: > No because use of what some people consider to be bad language isn't > necessarily abusive, offensive or degrading. Our most heavily censored > medium is TV and "fuck" is now considered acceptable in certain > contexts on most channels in the UK and EU. This makes following the CoC extremely hard to a non-native speaker as it is not too explicit on what is OK and what is not. I did through the whole thing with an eye glass and this what I deduced from it. /Jarkko
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 01:48:08PM -0800, David Miller wrote: > From: Jarkko Sakkinen > Date: Fri, 30 Nov 2018 13:44:05 -0800 > > > On Fri, Nov 30, 2018 at 01:01:02PM -0800, James Bottomley wrote: > >> No because use of what some people consider to be bad language isn't > >> necessarily abusive, offensive or degrading. Our most heavily censored > >> medium is TV and "fuck" is now considered acceptable in certain > >> contexts on most channels in the UK and EU. > > > > This makes following the CoC extremely hard to a non-native speaker as > > it is not too explicit on what is OK and what is not. I did through the > > whole thing with an eye glass and this what I deduced from it. > > It would be helpful if you could explain what part of the language > is unclear wrt. explaining how CoC does not apply to existing code. > > That part seems very explicit to me. Well, now that I re-read it, it was this part to be exact: "Maintainers have the right and responsibility to remove, edit, or reject comments, commits, code, wiki edits, issues, and other contributions that are not aligned to this Code of Conduct, or to ban temporarily or permanently any contributor for other behaviors that they deem inappropriate, threatening, offensive, or harmful." How this should be interpreted? I have not really followed the previous CoC discussions as I try to always use polite language so I'm sorry if this duplicate. /Jarkko
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 01:57:49PM -0800, James Bottomley wrote: > On Fri, 2018-11-30 at 13:44 -0800, Jarkko Sakkinen wrote: > > On Fri, Nov 30, 2018 at 01:01:02PM -0800, James Bottomley wrote: > > > No because use of what some people consider to be bad language > > > isn't necessarily abusive, offensive or degrading. Our most > > > heavily censored medium is TV and "fuck" is now considered > > > acceptable in certain contexts on most channels in the UK and EU. > > > > This makes following the CoC extremely hard to a non-native speaker > > as it is not too explicit on what is OK and what is not. I did > > through the whole thing with an eye glass and this what I deduced > > from it. > > OK, so something that would simply be considered in some quarters as > bad language isn't explicitly banned. The thing which differentiates > simple bad language from "abusive, offensive or degrading language", > which is called out by the CoC, is the context and the target. > > So when it's a simple expletive or the code of the author or even the > hardware is the target, I'd say it's an easy determination it's not a > CoC violation. If someone else's code is the target or the inventor of > the hardware is targetted by name, I'd say it is. Even non-native > English speakers should be able to determine target and context, > because that's the essence of meaning. I pasted this already to another response and this was probably the part that ignited me to send the patch set (was a few days ago, so had to revisit to find the exact paragraph): "Maintainers have the right and responsibility to remove, edit, or reject comments, commits, code, wiki edits, issues, and other contributions that are not aligned to this Code of Conduct, or to ban temporarily or permanently any contributor for other behaviors that they deem inappropriate, threatening, offensive, or harmful." The whole patch set is neither a joke/troll nor something I would necessarily want to be include myself. It does have the RFC tag. As a maintainer myself (and based on somewhat disturbed feedback from other maintainers) I can only make the conclusion that nobody knows what the responsibility part here means. I would interpret, if I read it like at lawyer at least, that even for existing code you would need to do the changes postmorterm. Is this wrong interpretation? Should I conclude that I made a mistake by reading the CoC and trying to understand what it *actually* says? After this discussion, I can say that I understand it less than before. /Jarkko
