* [PATCH] tpm: of: avoid __va() translation for event log address @ 2020-09-22 9:41 Ard Biesheuvel 2020-09-25 5:56 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Ard Biesheuvel @ 2020-09-22 9:41 UTC (permalink / raw) To: linux-integrity Cc: linux-kernel, Ard Biesheuvel, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe The TPM event log is provided to the OS by the firmware, by loading it into an area in memory and passing the physical address via a node in the device tree. Currently, we use __va() to access the memory via the kernel's linear map: however, it is not guaranteed that the linear map covers this particular address, as we may be running under HIGHMEM on a 32-bit architecture, or running firmware that uses a memory type for the event log that is omitted from the linear map (such as EfiReserved). So instead, use memremap(), which will reuse the linear mapping if it is valid, or create another mapping otherwise. Cc: Peter Huewe <peterhuewe@gmx.de> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Cc: Jason Gunthorpe <jgg@ziepe.ca> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/char/tpm/eventlog/of.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c index a9ce66d09a75..9178547589a3 100644 --- a/drivers/char/tpm/eventlog/of.c +++ b/drivers/char/tpm/eventlog/of.c @@ -11,6 +11,7 @@ */ #include <linux/slab.h> +#include <linux/io.h> #include <linux/of.h> #include <linux/tpm_eventlog.h> @@ -25,6 +26,7 @@ int tpm_read_log_of(struct tpm_chip *chip) struct tpm_bios_log *log; u32 size; u64 base; + void *p; log = &chip->log; if (chip->dev.parent && chip->dev.parent->of_node) @@ -65,7 +67,11 @@ int tpm_read_log_of(struct tpm_chip *chip) return -EIO; } - log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL); + p = memremap(base, size, MEMREMAP_WB); + if (!p) + return -ENOMEM; + log->bios_event_log = kmemdup(p, size, GFP_KERNEL); + memunmap(p); if (!log->bios_event_log) return -ENOMEM; -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-22 9:41 [PATCH] tpm: of: avoid __va() translation for event log address Ard Biesheuvel @ 2020-09-25 5:56 ` Jarkko Sakkinen 2020-09-25 5:57 ` Jarkko Sakkinen 2020-09-25 7:00 ` Ard Biesheuvel 0 siblings, 2 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2020-09-25 5:56 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-integrity, linux-kernel, Peter Huewe, Jason Gunthorpe On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > The TPM event log is provided to the OS by the firmware, by loading > it into an area in memory and passing the physical address via a node > in the device tree. > > Currently, we use __va() to access the memory via the kernel's linear > map: however, it is not guaranteed that the linear map covers this > particular address, as we may be running under HIGHMEM on a 32-bit > architecture, or running firmware that uses a memory type for the > event log that is omitted from the linear map (such as EfiReserved). Makes perfect sense to the level that I wonder if this should have a fixes tag and/or needs to be backported to the stable kernels? > So instead, use memremap(), which will reuse the linear mapping if > it is valid, or create another mapping otherwise. > > Cc: Peter Huewe <peterhuewe@gmx.de> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > drivers/char/tpm/eventlog/of.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c > index a9ce66d09a75..9178547589a3 100644 > --- a/drivers/char/tpm/eventlog/of.c > +++ b/drivers/char/tpm/eventlog/of.c > @@ -11,6 +11,7 @@ > */ > > #include <linux/slab.h> > +#include <linux/io.h> > #include <linux/of.h> > #include <linux/tpm_eventlog.h> > > @@ -25,6 +26,7 @@ int tpm_read_log_of(struct tpm_chip *chip) > struct tpm_bios_log *log; > u32 size; > u64 base; > + void *p; I'd just use 'ptr' for readability sake. > log = &chip->log; > if (chip->dev.parent && chip->dev.parent->of_node) > @@ -65,7 +67,11 @@ int tpm_read_log_of(struct tpm_chip *chip) > return -EIO; > } > > - log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL); > + p = memremap(base, size, MEMREMAP_WB); > + if (!p) > + return -ENOMEM; > + log->bios_event_log = kmemdup(p, size, GFP_KERNEL); > + memunmap(p); > if (!log->bios_event_log) > return -ENOMEM; > > -- > 2.17.1 > This is a really great catch! I'm a bit late of my PR a bit because of SGX upstreaming madness (sending v39 soon). If you can answer to my question above, I can do that nitpick change to patch and get it to my v5.10 PR. PS. Just so that you know, once I've applied it, it will be available here: git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git I'll include MAINTAINERS update to that PR. /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-25 5:56 ` Jarkko Sakkinen @ 2020-09-25 5:57 ` Jarkko Sakkinen 2020-09-25 7:00 ` Ard Biesheuvel 1 sibling, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2020-09-25 5:57 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-integrity, linux-kernel, Peter Huewe, Jason Gunthorpe On Fri, Sep 25, 2020 at 08:56:30AM +0300, Jarkko Sakkinen wrote: > On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > The TPM event log is provided to the OS by the firmware, by loading > > it into an area in memory and passing the physical address via a node > > in the device tree. > > > > Currently, we use __va() to access the memory via the kernel's linear > > map: however, it is not guaranteed that the linear map covers this > > particular address, as we may be running under HIGHMEM on a 32-bit > > architecture, or running firmware that uses a memory type for the > > event log that is omitted from the linear map (such as EfiReserved). > > Makes perfect sense to the level that I wonder if this should have a > fixes tag and/or needs to be backported to the stable kernels? > > > So instead, use memremap(), which will reuse the linear mapping if > > it is valid, or create another mapping otherwise. > > > > Cc: Peter Huewe <peterhuewe@gmx.de> > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > drivers/char/tpm/eventlog/of.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c > > index a9ce66d09a75..9178547589a3 100644 > > --- a/drivers/char/tpm/eventlog/of.c > > +++ b/drivers/char/tpm/eventlog/of.c > > @@ -11,6 +11,7 @@ > > */ > > > > #include <linux/slab.h> > > +#include <linux/io.h> > > #include <linux/of.h> > > #include <linux/tpm_eventlog.h> > > > > @@ -25,6 +26,7 @@ int tpm_read_log_of(struct tpm_chip *chip) > > struct tpm_bios_log *log; > > u32 size; > > u64 base; > > + void *p; > > I'd just use 'ptr' for readability sake. > > > log = &chip->log; > > if (chip->dev.parent && chip->dev.parent->of_node) > > @@ -65,7 +67,11 @@ int tpm_read_log_of(struct tpm_chip *chip) > > return -EIO; > > } > > > > - log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL); > > + p = memremap(base, size, MEMREMAP_WB); > > + if (!p) > > + return -ENOMEM; > > + log->bios_event_log = kmemdup(p, size, GFP_KERNEL); > > + memunmap(p); > > if (!log->bios_event_log) > > return -ENOMEM; > > > > -- > > 2.17.1 > > > > This is a really great catch! > > I'm a bit late of my PR a bit because of SGX upstreaming madness > (sending v39 soon). If you can answer to my question above, I can do > that nitpick change to patch and get it to my v5.10 PR. > > PS. Just so that you know, once I've applied it, it will be available > here: > > git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git > > I'll include MAINTAINERS update to that PR. Forgot this: Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-25 5:56 ` Jarkko Sakkinen 2020-09-25 5:57 ` Jarkko Sakkinen @ 2020-09-25 7:00 ` Ard Biesheuvel 2020-09-25 10:29 ` Jarkko Sakkinen 1 sibling, 1 reply; 13+ messages in thread From: Ard Biesheuvel @ 2020-09-25 7:00 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, Linux Kernel Mailing List, Peter Huewe, Jason Gunthorpe On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > The TPM event log is provided to the OS by the firmware, by loading > > it into an area in memory and passing the physical address via a node > > in the device tree. > > > > Currently, we use __va() to access the memory via the kernel's linear > > map: however, it is not guaranteed that the linear map covers this > > particular address, as we may be running under HIGHMEM on a 32-bit > > architecture, or running firmware that uses a memory type for the > > event log that is omitted from the linear map (such as EfiReserved). > > Makes perfect sense to the level that I wonder if this should have a > fixes tag and/or needs to be backported to the stable kernels? > AIUI, the code was written specifically for ppc64, which is a non-highmem, non-EFI architecture. However, when we start reusing this driver for ARM, this issue could pop up. The code itself has been refactored a couple of times, so I think it will require different versions of the patch for different generations of stable kernels. So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how far back it applies cleanly? > > So instead, use memremap(), which will reuse the linear mapping if > > it is valid, or create another mapping otherwise. > > > > Cc: Peter Huewe <peterhuewe@gmx.de> > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > drivers/char/tpm/eventlog/of.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c > > index a9ce66d09a75..9178547589a3 100644 > > --- a/drivers/char/tpm/eventlog/of.c > > +++ b/drivers/char/tpm/eventlog/of.c > > @@ -11,6 +11,7 @@ > > */ > > > > #include <linux/slab.h> > > +#include <linux/io.h> > > #include <linux/of.h> > > #include <linux/tpm_eventlog.h> > > > > @@ -25,6 +26,7 @@ int tpm_read_log_of(struct tpm_chip *chip) > > struct tpm_bios_log *log; > > u32 size; > > u64 base; > > + void *p; > > I'd just use 'ptr' for readability sake. > If you prefer > > log = &chip->log; > > if (chip->dev.parent && chip->dev.parent->of_node) > > @@ -65,7 +67,11 @@ int tpm_read_log_of(struct tpm_chip *chip) > > return -EIO; > > } > > > > - log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL); > > + p = memremap(base, size, MEMREMAP_WB); > > + if (!p) > > + return -ENOMEM; > > + log->bios_event_log = kmemdup(p, size, GFP_KERNEL); > > + memunmap(p); > > if (!log->bios_event_log) > > return -ENOMEM; > > > > -- > > 2.17.1 > > > > This is a really great catch! > > I'm a bit late of my PR a bit because of SGX upstreaming madness > (sending v39 soon). If you can answer to my question above, I can do > that nitpick change to patch and get it to my v5.10 PR. > Yes, please. > PS. Just so that you know, once I've applied it, it will be available > here: > > git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git > > I'll include MAINTAINERS update to that PR. > > /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-25 7:00 ` Ard Biesheuvel @ 2020-09-25 10:29 ` Jarkko Sakkinen 2020-09-25 12:00 ` Jason Gunthorpe 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2020-09-25 10:29 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-integrity, Linux Kernel Mailing List, Peter Huewe, Jason Gunthorpe On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> wrote: > > > > On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > > The TPM event log is provided to the OS by the firmware, by loading > > > it into an area in memory and passing the physical address via a node > > > in the device tree. > > > > > > Currently, we use __va() to access the memory via the kernel's linear > > > map: however, it is not guaranteed that the linear map covers this > > > particular address, as we may be running under HIGHMEM on a 32-bit > > > architecture, or running firmware that uses a memory type for the > > > event log that is omitted from the linear map (such as EfiReserved). > > > > Makes perfect sense to the level that I wonder if this should have a > > fixes tag and/or needs to be backported to the stable kernels? > > > > AIUI, the code was written specifically for ppc64, which is a > non-highmem, non-EFI architecture. However, when we start reusing this > driver for ARM, this issue could pop up. > > The code itself has been refactored a couple of times, so I think it > will require different versions of the patch for different generations > of stable kernels. > > So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how > far back it applies cleanly? Yeah, I think I'll cc it with some note before the diffstat. I'm thinking to cap it to only 5.x kernels (at least first) unless it is dead easy to backport below that. > > This is a really great catch! > > > > I'm a bit late of my PR a bit because of SGX upstreaming madness > > (sending v39 soon). If you can answer to my question above, I can do > > that nitpick change to patch and get it to my v5.10 PR. > > > > Yes, please. Great, will do, thanks again for fixing this issue! /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-25 10:29 ` Jarkko Sakkinen @ 2020-09-25 12:00 ` Jason Gunthorpe 2020-09-27 23:44 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Jason Gunthorpe @ 2020-09-25 12:00 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Ard Biesheuvel, linux-integrity, Linux Kernel Mailing List, Peter Huewe On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > <jarkko.sakkinen@linux.intel.com> wrote: > > > > > > On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > > > The TPM event log is provided to the OS by the firmware, by loading > > > > it into an area in memory and passing the physical address via a node > > > > in the device tree. > > > > > > > > Currently, we use __va() to access the memory via the kernel's linear > > > > map: however, it is not guaranteed that the linear map covers this > > > > particular address, as we may be running under HIGHMEM on a 32-bit > > > > architecture, or running firmware that uses a memory type for the > > > > event log that is omitted from the linear map (such as EfiReserved). > > > > > > Makes perfect sense to the level that I wonder if this should have a > > > fixes tag and/or needs to be backported to the stable kernels? > > > > > > > AIUI, the code was written specifically for ppc64, which is a > > non-highmem, non-EFI architecture. However, when we start reusing this > > driver for ARM, this issue could pop up. > > > > The code itself has been refactored a couple of times, so I think it > > will require different versions of the patch for different generations > > of stable kernels. > > > > So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how > > far back it applies cleanly? > > Yeah, I think I'll cc it with some note before the diffstat. > > I'm thinking to cap it to only 5.x kernels (at least first) unless it is > dead easy to backport below that. I have this vauge recollection of pointing at this before and being told that it had to be __va for some PPC reason? Do check with the PPC people first, I see none on the CC list. Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-25 12:00 ` Jason Gunthorpe @ 2020-09-27 23:44 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2020-09-27 23:44 UTC (permalink / raw) To: Jason Gunthorpe, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras Cc: Ard Biesheuvel, linux-integrity, Linux Kernel Mailing List, Peter Huewe, linuxppc-dev On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > > On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > > On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > > <jarkko.sakkinen@linux.intel.com> wrote: > > > > > > > > On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > > > > The TPM event log is provided to the OS by the firmware, by loading > > > > > it into an area in memory and passing the physical address via a node > > > > > in the device tree. > > > > > > > > > > Currently, we use __va() to access the memory via the kernel's linear > > > > > map: however, it is not guaranteed that the linear map covers this > > > > > particular address, as we may be running under HIGHMEM on a 32-bit > > > > > architecture, or running firmware that uses a memory type for the > > > > > event log that is omitted from the linear map (such as EfiReserved). > > > > > > > > Makes perfect sense to the level that I wonder if this should have a > > > > fixes tag and/or needs to be backported to the stable kernels? > > > > > > > > > > AIUI, the code was written specifically for ppc64, which is a > > > non-highmem, non-EFI architecture. However, when we start reusing this > > > driver for ARM, this issue could pop up. > > > > > > The code itself has been refactored a couple of times, so I think it > > > will require different versions of the patch for different generations > > > of stable kernels. > > > > > > So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how > > > far back it applies cleanly? > > > > Yeah, I think I'll cc it with some note before the diffstat. > > > > I'm thinking to cap it to only 5.x kernels (at least first) unless it is > > dead easy to backport below that. > > I have this vauge recollection of pointing at this before and being > told that it had to be __va for some PPC reason? > > Do check with the PPC people first, I see none on the CC list. > > Jason Thanks, added arch/powerpc maintainers. /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address @ 2020-09-27 23:44 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2020-09-27 23:44 UTC (permalink / raw) To: Jason Gunthorpe, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras Cc: linux-integrity, linuxppc-dev, Ard Biesheuvel, Peter Huewe, Linux Kernel Mailing List On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > > On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > > On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > > <jarkko.sakkinen@linux.intel.com> wrote: > > > > > > > > On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > > > > The TPM event log is provided to the OS by the firmware, by loading > > > > > it into an area in memory and passing the physical address via a node > > > > > in the device tree. > > > > > > > > > > Currently, we use __va() to access the memory via the kernel's linear > > > > > map: however, it is not guaranteed that the linear map covers this > > > > > particular address, as we may be running under HIGHMEM on a 32-bit > > > > > architecture, or running firmware that uses a memory type for the > > > > > event log that is omitted from the linear map (such as EfiReserved). > > > > > > > > Makes perfect sense to the level that I wonder if this should have a > > > > fixes tag and/or needs to be backported to the stable kernels? > > > > > > > > > > AIUI, the code was written specifically for ppc64, which is a > > > non-highmem, non-EFI architecture. However, when we start reusing this > > > driver for ARM, this issue could pop up. > > > > > > The code itself has been refactored a couple of times, so I think it > > > will require different versions of the patch for different generations > > > of stable kernels. > > > > > > So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how > > > far back it applies cleanly? > > > > Yeah, I think I'll cc it with some note before the diffstat. > > > > I'm thinking to cap it to only 5.x kernels (at least first) unless it is > > dead easy to backport below that. > > I have this vauge recollection of pointing at this before and being > told that it had to be __va for some PPC reason? > > Do check with the PPC people first, I see none on the CC list. > > Jason Thanks, added arch/powerpc maintainers. /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-27 23:44 ` Jarkko Sakkinen (?) @ 2020-09-28 5:56 ` Christophe Leroy 2020-09-28 6:20 ` Ard Biesheuvel -1 siblings, 1 reply; 13+ messages in thread From: Christophe Leroy @ 2020-09-28 5:56 UTC (permalink / raw) To: Jarkko Sakkinen, Jason Gunthorpe, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras Cc: linux-integrity, linuxppc-dev, Ard Biesheuvel, Peter Huewe, Linux Kernel Mailing List Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit : > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen >>>> <jarkko.sakkinen@linux.intel.com> wrote: >>>>> >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: >>>>>> The TPM event log is provided to the OS by the firmware, by loading >>>>>> it into an area in memory and passing the physical address via a node >>>>>> in the device tree. >>>>>> >>>>>> Currently, we use __va() to access the memory via the kernel's linear >>>>>> map: however, it is not guaranteed that the linear map covers this >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit >>>>>> architecture, or running firmware that uses a memory type for the >>>>>> event log that is omitted from the linear map (such as EfiReserved). >>>>> >>>>> Makes perfect sense to the level that I wonder if this should have a >>>>> fixes tag and/or needs to be backported to the stable kernels? >>>>> >>>> >>>> AIUI, the code was written specifically for ppc64, which is a >>>> non-highmem, non-EFI architecture. However, when we start reusing this >>>> driver for ARM, this issue could pop up. >>>> >>>> The code itself has been refactored a couple of times, so I think it >>>> will require different versions of the patch for different generations >>>> of stable kernels. >>>> >>>> So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how >>>> far back it applies cleanly? >>> >>> Yeah, I think I'll cc it with some note before the diffstat. >>> >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is >>> dead easy to backport below that. >> >> I have this vauge recollection of pointing at this before and being >> told that it had to be __va for some PPC reason? >> >> Do check with the PPC people first, I see none on the CC list. >> >> Jason > > Thanks, added arch/powerpc maintainers. > As far as I can see, memremap() won't work on PPC32 at least: IIUC, memremap() calls arch_memremap_wb() arch_memremap_wb() calls ioremap_cache() In case of failure, then ioremap_wt() and ioremap_wc() are tried. All ioremap calls end up in __ioremap_caller() which will return NULL in case you try to ioremap RAM. So the statement "So instead, use memremap(), which will reuse the linear mapping if it is valid, or create another mapping otherwise." seems to be wrong, at least for PPC32. Even for PPC64 which doesn't seem to have the RAM check, I can't see that it will "reuse the linear mapping". Christophe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-28 5:56 ` Christophe Leroy @ 2020-09-28 6:20 ` Ard Biesheuvel 0 siblings, 0 replies; 13+ messages in thread From: Ard Biesheuvel @ 2020-09-28 6:20 UTC (permalink / raw) To: Christophe Leroy Cc: Jarkko Sakkinen, Jason Gunthorpe, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linux-integrity, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Peter Huewe, Linux Kernel Mailing List On Mon, 28 Sep 2020 at 07:56, Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit : > > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > >>>> <jarkko.sakkinen@linux.intel.com> wrote: > >>>>> > >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > >>>>>> The TPM event log is provided to the OS by the firmware, by loading > >>>>>> it into an area in memory and passing the physical address via a node > >>>>>> in the device tree. > >>>>>> > >>>>>> Currently, we use __va() to access the memory via the kernel's linear > >>>>>> map: however, it is not guaranteed that the linear map covers this > >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit > >>>>>> architecture, or running firmware that uses a memory type for the > >>>>>> event log that is omitted from the linear map (such as EfiReserved). > >>>>> > >>>>> Makes perfect sense to the level that I wonder if this should have a > >>>>> fixes tag and/or needs to be backported to the stable kernels? > >>>>> > >>>> > >>>> AIUI, the code was written specifically for ppc64, which is a > >>>> non-highmem, non-EFI architecture. However, when we start reusing this > >>>> driver for ARM, this issue could pop up. > >>>> > >>>> The code itself has been refactored a couple of times, so I think it > >>>> will require different versions of the patch for different generations > >>>> of stable kernels. > >>>> > >>>> So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how > >>>> far back it applies cleanly? > >>> > >>> Yeah, I think I'll cc it with some note before the diffstat. > >>> > >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is > >>> dead easy to backport below that. > >> > >> I have this vauge recollection of pointing at this before and being > >> told that it had to be __va for some PPC reason? > >> > >> Do check with the PPC people first, I see none on the CC list. > >> > >> Jason > > > > Thanks, added arch/powerpc maintainers. > > > > As far as I can see, memremap() won't work on PPC32 at least: > > IIUC, memremap() calls arch_memremap_wb() > arch_memremap_wb() calls ioremap_cache() > In case of failure, then ioremap_wt() and ioremap_wc() are tried. > > All ioremap calls end up in __ioremap_caller() which will return NULL in case you try to ioremap RAM. > > So the statement "So instead, use memremap(), which will reuse the linear mapping if > it is valid, or create another mapping otherwise." seems to be wrong, at least for PPC32. > > Even for PPC64 which doesn't seem to have the RAM check, I can't see that it will "reuse the linear > mapping". > It is there, please look again. Before any of the above happens, memremap() will call try_ram_remap() for regions that are covered by a IORESOURCE_SYSTEM_RAM, and map it using __va() if its PFN is valid and it is not highmem. So as far as I can tell, this change has no effect on PPC at all unless its RAM is not described as IORESOURCE_SYSTEM_RAM. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address @ 2020-09-28 6:20 ` Ard Biesheuvel 0 siblings, 0 replies; 13+ messages in thread From: Ard Biesheuvel @ 2020-09-28 6:20 UTC (permalink / raw) To: Christophe Leroy Cc: Linux Kernel Mailing List, Jarkko Sakkinen, Jason Gunthorpe, Paul Mackerras, linux-integrity, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Peter Huewe On Mon, 28 Sep 2020 at 07:56, Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit : > > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > >>>> <jarkko.sakkinen@linux.intel.com> wrote: > >>>>> > >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > >>>>>> The TPM event log is provided to the OS by the firmware, by loading > >>>>>> it into an area in memory and passing the physical address via a node > >>>>>> in the device tree. > >>>>>> > >>>>>> Currently, we use __va() to access the memory via the kernel's linear > >>>>>> map: however, it is not guaranteed that the linear map covers this > >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit > >>>>>> architecture, or running firmware that uses a memory type for the > >>>>>> event log that is omitted from the linear map (such as EfiReserved). > >>>>> > >>>>> Makes perfect sense to the level that I wonder if this should have a > >>>>> fixes tag and/or needs to be backported to the stable kernels? > >>>>> > >>>> > >>>> AIUI, the code was written specifically for ppc64, which is a > >>>> non-highmem, non-EFI architecture. However, when we start reusing this > >>>> driver for ARM, this issue could pop up. > >>>> > >>>> The code itself has been refactored a couple of times, so I think it > >>>> will require different versions of the patch for different generations > >>>> of stable kernels. > >>>> > >>>> So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how > >>>> far back it applies cleanly? > >>> > >>> Yeah, I think I'll cc it with some note before the diffstat. > >>> > >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is > >>> dead easy to backport below that. > >> > >> I have this vauge recollection of pointing at this before and being > >> told that it had to be __va for some PPC reason? > >> > >> Do check with the PPC people first, I see none on the CC list. > >> > >> Jason > > > > Thanks, added arch/powerpc maintainers. > > > > As far as I can see, memremap() won't work on PPC32 at least: > > IIUC, memremap() calls arch_memremap_wb() > arch_memremap_wb() calls ioremap_cache() > In case of failure, then ioremap_wt() and ioremap_wc() are tried. > > All ioremap calls end up in __ioremap_caller() which will return NULL in case you try to ioremap RAM. > > So the statement "So instead, use memremap(), which will reuse the linear mapping if > it is valid, or create another mapping otherwise." seems to be wrong, at least for PPC32. > > Even for PPC64 which doesn't seem to have the RAM check, I can't see that it will "reuse the linear > mapping". > It is there, please look again. Before any of the above happens, memremap() will call try_ram_remap() for regions that are covered by a IORESOURCE_SYSTEM_RAM, and map it using __va() if its PFN is valid and it is not highmem. So as far as I can tell, this change has no effect on PPC at all unless its RAM is not described as IORESOURCE_SYSTEM_RAM. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address 2020-09-28 6:20 ` Ard Biesheuvel @ 2020-09-28 14:09 ` Jarkko Sakkinen -1 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2020-09-28 14:09 UTC (permalink / raw) To: Ard Biesheuvel Cc: Christophe Leroy, Jason Gunthorpe, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, linux-integrity, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Peter Huewe, Linux Kernel Mailing List On Mon, Sep 28, 2020 at 08:20:18AM +0200, Ard Biesheuvel wrote: > On Mon, 28 Sep 2020 at 07:56, Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: > > > > > > > > Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit : > > > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > > >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > > >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > >>>> <jarkko.sakkinen@linux.intel.com> wrote: > > >>>>> > > >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > >>>>>> The TPM event log is provided to the OS by the firmware, by loading > > >>>>>> it into an area in memory and passing the physical address via a node > > >>>>>> in the device tree. > > >>>>>> > > >>>>>> Currently, we use __va() to access the memory via the kernel's linear > > >>>>>> map: however, it is not guaranteed that the linear map covers this > > >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit > > >>>>>> architecture, or running firmware that uses a memory type for the > > >>>>>> event log that is omitted from the linear map (such as EfiReserved). > > >>>>> > > >>>>> Makes perfect sense to the level that I wonder if this should have a > > >>>>> fixes tag and/or needs to be backported to the stable kernels? > > >>>>> > > >>>> > > >>>> AIUI, the code was written specifically for ppc64, which is a > > >>>> non-highmem, non-EFI architecture. However, when we start reusing this > > >>>> driver for ARM, this issue could pop up. > > >>>> > > >>>> The code itself has been refactored a couple of times, so I think it > > >>>> will require different versions of the patch for different generations > > >>>> of stable kernels. > > >>>> > > >>>> So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how > > >>>> far back it applies cleanly? > > >>> > > >>> Yeah, I think I'll cc it with some note before the diffstat. > > >>> > > >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is > > >>> dead easy to backport below that. > > >> > > >> I have this vauge recollection of pointing at this before and being > > >> told that it had to be __va for some PPC reason? > > >> > > >> Do check with the PPC people first, I see none on the CC list. > > >> > > >> Jason > > > > > > Thanks, added arch/powerpc maintainers. > > > > > > > As far as I can see, memremap() won't work on PPC32 at least: > > > > IIUC, memremap() calls arch_memremap_wb() > > arch_memremap_wb() calls ioremap_cache() > > In case of failure, then ioremap_wt() and ioremap_wc() are tried. > > > > All ioremap calls end up in __ioremap_caller() which will return NULL in case you try to ioremap RAM. > > > > So the statement "So instead, use memremap(), which will reuse the linear mapping if > > it is valid, or create another mapping otherwise." seems to be wrong, at least for PPC32. > > > > Even for PPC64 which doesn't seem to have the RAM check, I can't see that it will "reuse the linear > > mapping". > > > > It is there, please look again. Before any of the above happens, > memremap() will call try_ram_remap() for regions that are covered by a > IORESOURCE_SYSTEM_RAM, and map it using __va() if its PFN is valid and > it is not highmem. > > So as far as I can tell, this change has no effect on PPC at all > unless its RAM is not described as IORESOURCE_SYSTEM_RAM. Any chance for someone to test this on PPC32? /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: of: avoid __va() translation for event log address @ 2020-09-28 14:09 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2020-09-28 14:09 UTC (permalink / raw) To: Ard Biesheuvel Cc: Linux Kernel Mailing List, Jason Gunthorpe, Paul Mackerras, linux-integrity, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Peter Huewe On Mon, Sep 28, 2020 at 08:20:18AM +0200, Ard Biesheuvel wrote: > On Mon, 28 Sep 2020 at 07:56, Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: > > > > > > > > Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit : > > > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > > >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > > >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > >>>> <jarkko.sakkinen@linux.intel.com> wrote: > > >>>>> > > >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > >>>>>> The TPM event log is provided to the OS by the firmware, by loading > > >>>>>> it into an area in memory and passing the physical address via a node > > >>>>>> in the device tree. > > >>>>>> > > >>>>>> Currently, we use __va() to access the memory via the kernel's linear > > >>>>>> map: however, it is not guaranteed that the linear map covers this > > >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit > > >>>>>> architecture, or running firmware that uses a memory type for the > > >>>>>> event log that is omitted from the linear map (such as EfiReserved). > > >>>>> > > >>>>> Makes perfect sense to the level that I wonder if this should have a > > >>>>> fixes tag and/or needs to be backported to the stable kernels? > > >>>>> > > >>>> > > >>>> AIUI, the code was written specifically for ppc64, which is a > > >>>> non-highmem, non-EFI architecture. However, when we start reusing this > > >>>> driver for ARM, this issue could pop up. > > >>>> > > >>>> The code itself has been refactored a couple of times, so I think it > > >>>> will require different versions of the patch for different generations > > >>>> of stable kernels. > > >>>> > > >>>> So perhaps just add Cc: <stable@vger.kernel.org>, and wait and see how > > >>>> far back it applies cleanly? > > >>> > > >>> Yeah, I think I'll cc it with some note before the diffstat. > > >>> > > >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is > > >>> dead easy to backport below that. > > >> > > >> I have this vauge recollection of pointing at this before and being > > >> told that it had to be __va for some PPC reason? > > >> > > >> Do check with the PPC people first, I see none on the CC list. > > >> > > >> Jason > > > > > > Thanks, added arch/powerpc maintainers. > > > > > > > As far as I can see, memremap() won't work on PPC32 at least: > > > > IIUC, memremap() calls arch_memremap_wb() > > arch_memremap_wb() calls ioremap_cache() > > In case of failure, then ioremap_wt() and ioremap_wc() are tried. > > > > All ioremap calls end up in __ioremap_caller() which will return NULL in case you try to ioremap RAM. > > > > So the statement "So instead, use memremap(), which will reuse the linear mapping if > > it is valid, or create another mapping otherwise." seems to be wrong, at least for PPC32. > > > > Even for PPC64 which doesn't seem to have the RAM check, I can't see that it will "reuse the linear > > mapping". > > > > It is there, please look again. Before any of the above happens, > memremap() will call try_ram_remap() for regions that are covered by a > IORESOURCE_SYSTEM_RAM, and map it using __va() if its PFN is valid and > it is not highmem. > > So as far as I can tell, this change has no effect on PPC at all > unless its RAM is not described as IORESOURCE_SYSTEM_RAM. Any chance for someone to test this on PPC32? /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-09-28 14:23 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-22 9:41 [PATCH] tpm: of: avoid __va() translation for event log address Ard Biesheuvel 2020-09-25 5:56 ` Jarkko Sakkinen 2020-09-25 5:57 ` Jarkko Sakkinen 2020-09-25 7:00 ` Ard Biesheuvel 2020-09-25 10:29 ` Jarkko Sakkinen 2020-09-25 12:00 ` Jason Gunthorpe 2020-09-27 23:44 ` Jarkko Sakkinen 2020-09-27 23:44 ` Jarkko Sakkinen 2020-09-28 5:56 ` Christophe Leroy 2020-09-28 6:20 ` Ard Biesheuvel 2020-09-28 6:20 ` Ard Biesheuvel 2020-09-28 14:09 ` Jarkko Sakkinen 2020-09-28 14:09 ` Jarkko Sakkinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.