lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 7 Dec 2020 02:07:57 +0000
From:   Peng Fan <peng.fan@....com>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        "Peng Fan (OSS)" <peng.fan@....nxp.com>
CC:     "ohad@...ery.com" <ohad@...ery.com>,
        "mathieu.poirier@...aro.org" <mathieu.poirier@...aro.org>,
        "o.rempel@...gutronix.de" <o.rempel@...gutronix.de>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        "linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Richard Zhu <hongxing.zhu@....com>
Subject: RE: [PATCH V3 1/7] remoteproc: elf: support platform specific memory
 hook

Hi Bjorn,

> Subject: Re: [PATCH V3 1/7] remoteproc: elf: support platform specific
> memory hook
> 
> On Fri 04 Dec 01:40 CST 2020, Peng Fan (OSS) wrote:
> 
> > From: Peng Fan <peng.fan@....com>
> >
> > To arm64, "dc      zva, dst" is used in memset.
> > Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> >
> > "If the memory region being zeroed is any type of Device memory, this
> > instruction can give an alignment fault which is prioritized in the
> > same way as other alignment faults that are determined by the memory
> > type."
> >
> > On i.MX platforms, when elf is loaded to onchip TCM area, the region
> > is ioremapped, so "dc zva, dst" will trigger abort. And ioremap_wc()
> > on i.MX not able to write correct data to TCM area.
> >
> > So we need to use io helpers, and extend the elf loader to support
> > platform specific memory functions.
> >
> > Acked-by: Richard Zhu <hongxing.zhu@....com>
> > Signed-off-by: Peng Fan <peng.fan@....com>
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@...aro.org>
> > ---
> >  drivers/remoteproc/remoteproc_elf_loader.c | 20
> ++++++++++++++++++--
> >  include/linux/remoteproc.h                 |  4 ++++
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> > b/drivers/remoteproc/remoteproc_elf_loader.c
> > index df68d87752e4..6cb71fe47261 100644
> > --- a/drivers/remoteproc/remoteproc_elf_loader.c
> > +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> > @@ -129,6 +129,22 @@ u64 rproc_elf_get_boot_addr(struct rproc *rproc,
> > const struct firmware *fw)  }
> EXPORT_SYMBOL(rproc_elf_get_boot_addr);
> >
> > +static void rproc_elf_memcpy(struct rproc *rproc, void *dest, const
> > +void *src, size_t count) {
> > +	if (!rproc->ops->elf_memcpy)
> > +		memcpy(dest, src, count);
> > +
> > +	rproc->ops->elf_memcpy(rproc, dest, src, count);
> 
> Looking at the current set of remoteproc drivers I get a feeling that we'll end
> up with a while bunch of functions that all just wraps memcpy_toio(). And the
> reason for this is that we are we're "abusing" the carveout to carry the
> __iomem pointer without keeping track of it.
> 
> And this is not the only time we're supposed to use an io-accessor, another
> example is rproc_copy_segment() in rproc_coredump.c
> 
> It also means that if a platform driver for some reason where to support both
> ioremap and normal carveouts the elf_memcpy op would be quite quirky.
> 
> 
> So I would prefer if we track the knowledge about void *va being a __iomem
> or not in the struct rproc_mem_entry and make rproc_da_to_va() return this
> information as well.
> 
> Then instead of extending the ops we can make this simply call memcpy or
> memcpy_toio() depending on this.

A draft proposal as below, are you ok with the approach?

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 46c2937ebea9..bbb6e0613c1b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -189,13 +189,13 @@ EXPORT_SYMBOL(rproc_va_to_pa);
  * here the output of the DMA API for the carveouts, which should be more
  * correct.
  */
-void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
+void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *iomem)
 {
        struct rproc_mem_entry *carveout;
        void *ptr = NULL;

        if (rproc->ops->da_to_va) {
-               ptr = rproc->ops->da_to_va(rproc, da, len);
+               ptr = rproc->ops->da_to_va(rproc, da, len, iomem);
                if (ptr)
                        goto out;
        }
@@ -217,6 +217,9 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)

                ptr = carveout->va + offset;

+               if (iomem)
+                       iomem = carveout->iomem;
+
                break;
        }

diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
index 34530dc20cb4..5ff9389e6319 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -153,18 +153,22 @@ static void rproc_copy_segment(struct rproc *rproc, void *dest,
                               size_t offset, size_t size)
 {
        void *ptr;
+       bool iomem;

        if (segment->dump) {
                segment->dump(rproc, segment, dest, offset, size);
        } else {
-               ptr = rproc_da_to_va(rproc, segment->da + offset, size);
+               ptr = rproc_da_to_va(rproc, segment->da + offset, size, &iomem);
                if (!ptr) {
                        dev_err(&rproc->dev,
                                "invalid copy request for segment %pad with offset %zu and size %zu)\n",
                                &segment->da, offset, size);
                        memset(dest, 0xff, size);
                } else {
-                       memcpy(dest, ptr, size);
+                       if (iomem)
+                               memcpy_fromio(dest, ptr, size);
+                       else
+                               memcpy(dest, ptr, size);
                }
        }
 }
diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index df68d87752e4..20538143249e 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -175,6 +175,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
                u64 offset = elf_phdr_get_p_offset(class, phdr);
                u32 type = elf_phdr_get_p_type(class, phdr);
                void *ptr;
+               bool iomem;

                if (type != PT_LOAD)
                        continue;
@@ -204,7 +205,7 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
                }

                /* grab the kernel address for this device address */
-               ptr = rproc_da_to_va(rproc, da, memsz);
+               ptr = rproc_da_to_va(rproc, da, memsz, &iomem);
                if (!ptr) {
                        dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
                                memsz);
@@ -213,8 +214,12 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
                }

                /* put the segment where the remote processor expects it */
-               if (filesz)
-                       memcpy(ptr, elf_data + offset, filesz);
+               if (filesz) {
+                       if (iomem)
+                               memcpy_fromio(ptr, elf_data + offset, filesz);
+                       else
+                               memcpy(ptr, elf_data + offset, filesz);
+               }

                /*
                 * Zero out remaining memory for this segment.
@@ -223,8 +228,12 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
                 * did this for us. albeit harmless, we may consider removing
                 * this.
                 */
-               if (memsz > filesz)
-                       memset(ptr + filesz, 0, memsz - filesz);
+               if (memsz > filesz) {
+                       if (iomem)
+                               memset_toio(ptr + filesz, 0, memsz - filesz);
+                       else
+                               memset(ptr + filesz, 0, memsz - filesz);
+               }
        }

        return ret;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e8ac041c64d9..01bb9fa12784 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -329,6 +329,7 @@ struct rproc;
  */
 struct rproc_mem_entry {
        void *va;
+       bool iomem;
        dma_addr_t dma;
        size_t len;
        u32 da;
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index d6473a72a336..dfa0bd7812a5 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -194,7 +194,7 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
 }

 static __always_inline unsigned long __must_check
-copy_to_user(void __user *to, const void *from, unsigned long n)
+copy_to_user(void __user *to, const void *_toiofrom, unsigned long n)
 {
        if (likely(check_copy_size(from, n, true)))
                n = _copy_to_user(to, from, n);

Thanks,
Peng.

> 
> Regards,
> Bjorn
> 
> > +}
> > +
> > +static void rproc_elf_memset(struct rproc *rproc, void *s, int c,
> > +size_t count) {
> > +	if (!rproc->ops->elf_memset)
> > +		memset(s, c, count);
> > +
> > +	rproc->ops->elf_memset(rproc, s, c, count); }
> > +
> >  /**
> >   * rproc_elf_load_segments() - load firmware segments to memory
> >   * @rproc: remote processor which will be booted using these fw
> > segments @@ -214,7 +230,7 @@ int rproc_elf_load_segments(struct rproc
> > *rproc, const struct firmware *fw)
> >
> >  		/* put the segment where the remote processor expects it */
> >  		if (filesz)
> > -			memcpy(ptr, elf_data + offset, filesz);
> > +			rproc_elf_memcpy(rproc, ptr, elf_data + offset, filesz);
> >
> >  		/*
> >  		 * Zero out remaining memory for this segment.
> > @@ -224,7 +240,7 @@ int rproc_elf_load_segments(struct rproc *rproc,
> const struct firmware *fw)
> >  		 * this.
> >  		 */
> >  		if (memsz > filesz)
> > -			memset(ptr + filesz, 0, memsz - filesz);
> > +			rproc_elf_memset(rproc, ptr + filesz, 0, memsz - filesz);
> >  	}
> >
> >  	return ret;
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index e8ac041c64d9..06c52f88a3fd 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -373,6 +373,8 @@ enum rsc_handling_status {
> >   *			expects to find it
> >   * @sanity_check:	sanity check the fw image
> >   * @get_boot_addr:	get boot address to entry point specified in
> firmware
> > + * @elf_memcpy:		platform specific elf loader memcpy
> > + * @elf_memset:		platform specific elf loader memset
> >   * @panic:	optional callback to react to system panic, core will delay
> >   *		panic at least the returned number of milliseconds
> >   */
> > @@ -392,6 +394,8 @@ struct rproc_ops {
> >  	int (*load)(struct rproc *rproc, const struct firmware *fw);
> >  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
> >  	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware
> > *fw);
> > +	void (*elf_memcpy)(struct rproc *rproc, void *dest, const void *src,
> size_t count);
> > +	void (*elf_memset)(struct rproc *rproc, void *s, int c, size_t
> > +count);
> >  	unsigned long (*panic)(struct rproc *rproc);  };
> >
> > --
> > 2.28.0
> >

Powered by blists - more mailing lists