All of lore.kernel.org
 help / color / mirror / Atom feed
From: Henri Roosen <henri.roosen@ginzinger.com>
To: linux-remoteproc@vger.kernel.org
Cc: Henri Roosen <henri.roosen@ginzinger.com>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: [PATCH 1/1] remoteproc: fix elf_loader da_to_va translation and writing beyond segment
Date: Wed, 3 May 2017 14:12:09 +0200	[thread overview]
Message-ID: <1493813529-19184-1-git-send-email-henri.roosen@ginzinger.com> (raw)

Consider a system with 2 memory regions:
0x1fff8000 - 0x1fffffff: iram
0x21000000 - 0x21007fff: dram

The .elf file for this system contains the following Program Headers:
Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R   0x4
  LOAD           0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10
  LOAD           0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW  0x4

 Section to Segment mapping:
  Segment Sections...
   00     .interrupts
   01     .text .ARM .init_array .fini_array
   02     .data .bss .heap .stack

The problem is with the 3rd segment: it has 0x1cc bytes of ROM .data
which need to be placed at PhysAddr 0x1fffbf5c. Using MemSiz as len
parameter for rproc_da_to_va is incorrect (goes beyond 0x1fffffff). The
correct len parameter to be used here is FileSiz.

The actual memcpy of the segment was already correctly using the FileSiz
for length, however the unnecessary "Zero out remaining memory" would
write beyond the 0x1fffffff end of the memory region! This patch removes
the harmful code.

Signed-off-by: Henri Roosen <henri.roosen@ginzinger.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index c523983..3fa159a 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -183,9 +183,10 @@ 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, filesz);
 		if (!ptr) {
-			dev_err(dev, "bad phdr da 0x%x mem 0x%x\n", da, memsz);
+			dev_err(dev, "bad phdr da 0x%x filesz 0x%x\n",
+				da, filesz);
 			ret = -EINVAL;
 			break;
 		}
@@ -193,16 +194,6 @@ rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 		/* put the segment where the remote processor expects it */
 		if (phdr->p_filesz)
 			memcpy(ptr, elf_data + phdr->p_offset, filesz);
-
-		/*
-		 * Zero out remaining memory for this segment.
-		 *
-		 * This isn't strictly required since dma_alloc_coherent already
-		 * did this for us. albeit harmless, we may consider removing
-		 * this.
-		 */
-		if (memsz > filesz)
-			memset(ptr + filesz, 0, memsz - filesz);
 	}
 
 	return ret;
-- 
2.1.4

WARNING: multiple messages have this Message-ID (diff)
From: Henri Roosen <henri.roosen@ginzinger.com>
To: <linux-remoteproc@vger.kernel.org>
Cc: Henri Roosen <henri.roosen@ginzinger.com>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: [PATCH 1/1] remoteproc: fix elf_loader da_to_va translation and writing beyond segment
Date: Wed, 3 May 2017 14:12:09 +0200	[thread overview]
Message-ID: <1493813529-19184-1-git-send-email-henri.roosen@ginzinger.com> (raw)

Consider a system with 2 memory regions:
0x1fff8000 - 0x1fffffff: iram
0x21000000 - 0x21007fff: dram

The .elf file for this system contains the following Program Headers:
Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000094 0x1fff8000 0x1fff8000 0x00240 0x00240 R   0x4
  LOAD           0x0002e0 0x1fff8240 0x1fff8240 0x03d1c 0x03d1c RWE 0x10
  LOAD           0x003ffc 0x21000000 0x1fffbf5c 0x001cc 0x048a0 RW  0x4

 Section to Segment mapping:
  Segment Sections...
   00     .interrupts
   01     .text .ARM .init_array .fini_array
   02     .data .bss .heap .stack

The problem is with the 3rd segment: it has 0x1cc bytes of ROM .data
which need to be placed at PhysAddr 0x1fffbf5c. Using MemSiz as len
parameter for rproc_da_to_va is incorrect (goes beyond 0x1fffffff). The
correct len parameter to be used here is FileSiz.

The actual memcpy of the segment was already correctly using the FileSiz
for length, however the unnecessary "Zero out remaining memory" would
write beyond the 0x1fffffff end of the memory region! This patch removes
the harmful code.

Signed-off-by: Henri Roosen <henri.roosen@ginzinger.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index c523983..3fa159a 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -183,9 +183,10 @@ 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, filesz);
 		if (!ptr) {
-			dev_err(dev, "bad phdr da 0x%x mem 0x%x\n", da, memsz);
+			dev_err(dev, "bad phdr da 0x%x filesz 0x%x\n",
+				da, filesz);
 			ret = -EINVAL;
 			break;
 		}
@@ -193,16 +194,6 @@ rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 		/* put the segment where the remote processor expects it */
 		if (phdr->p_filesz)
 			memcpy(ptr, elf_data + phdr->p_offset, filesz);
-
-		/*
-		 * Zero out remaining memory for this segment.
-		 *
-		 * This isn't strictly required since dma_alloc_coherent already
-		 * did this for us. albeit harmless, we may consider removing
-		 * this.
-		 */
-		if (memsz > filesz)
-			memset(ptr + filesz, 0, memsz - filesz);
 	}
 
 	return ret;
-- 
2.1.4

             reply	other threads:[~2017-05-03 12:12 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-03 12:12 Henri Roosen [this message]
2017-05-03 12:12 ` [PATCH 1/1] remoteproc: fix elf_loader da_to_va translation and writing beyond segment Henri Roosen
2017-05-11  0:05 ` Bjorn Andersson
2017-05-11 16:12   ` Henri Roosen
2017-05-11 16:12     ` Henri Roosen
2017-05-14  4:14     ` Bjorn Andersson
2017-05-15 14:37       ` Henri Roosen
2017-05-15 14:37         ` Henri Roosen
2017-05-18 18:00         ` Bjorn Andersson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1493813529-19184-1-git-send-email-henri.roosen@ginzinger.com \
    --to=henri.roosen@ginzinger.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=ohad@wizery.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.