Re: [PATCH v6 3/3] mtd: rawnand: Add support for secure regions in NAND memory
Hi Manivannan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on mtd/mtd/next]
[also build test WARNING on mtd/mtd/fixes mtd/nand/next v5.12-rc3 next-20210318]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Manivannan-Sadhasivam/Add-support-for-secure-regions-in-NAND/20210318-204636
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next
config: arm-randconfig-r023-20210318 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
6db3ab2903f42712f44000afb5aa467efbd25f35)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
#
https://github.com/0day-ci/linux/commit/045edb4991f99260412ca8ecbcbf1f41fcd30941
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Manivannan-Sadhasivam/Add-support-for-secure-regions-in-NAND/20210318-204636
git checkout 045edb4991f99260412ca8ecbcbf1f41fcd30941
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/mtd/nand/raw/nand_base.c:4255:52: warning: variable 'len' is
>> uninitialized when used here [-Wuninitialized]
ret = nand_check_secure_region(chip, instr->addr, len);
^~~
drivers/mtd/nand/raw/nand_base.c:4245:12: note: initialize the variable
'len' to silence this warning
loff_t len;
^
= 0
1 warning generated.
vim +/len +4255 drivers/mtd/nand/raw/nand_base.c
4232
4233 /**
4234 * nand_erase_nand - [INTERN] erase block(s)
4235 * @chip: NAND chip object
4236 * @instr: erase instruction
4237 * @allowbbt: allow erasing the bbt area
4238 *
4239 * Erase one ore more blocks.
4240 */
4241 int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
4242 int allowbbt)
4243 {
4244 int page, pages_per_block, ret, chipnr;
4245 loff_t len;
4246
4247 pr_debug("%s: start = 0x%012llx, len = %llu\n",
4248 __func__, (unsigned long long)instr->addr,
4249 (unsigned long long)instr->len);
4250
4251 if (check_offs_len(chip, instr->addr, instr->len))
4252 return -EINVAL;
4253
4254 /* Check if the region is secured */
> 4255 ret = nand_check_secure_region(chip, instr->addr, len);
4256 if (ret)
4257 return ret;
4258
4259 /* Grab the lock and see if the device is available */
4260 ret = nand_get_device(chip);
4261 if (ret)
4262 return ret;
4263
4264 /* Shift to get first page */
4265 page = (int)(instr->addr >> chip->page_shift);
4266 chipnr = (int)(instr->addr >> chip->chip_shift);
4267
4268 /* Calculate pages in each block */
4269 pages_per_block = 1 << (chip->phys_erase_shift -
chip->page_shift);
4270
4271 /* Select the NAND device */
4272 nand_select_target(chip, chipnr);
4273
4274 /* Check, if it is write protected */
4275 if (nand_check_wp(chip)) {
4276 pr_debug("%s: device is write protected!\n",
4277 __func__);
4278 ret = -EIO;
4279 goto erase_exit;
4280 }
4281
4282 /* Loop through the pages */
4283 len = instr->len;
4284
4285 while (len) {
4286 /* Check if we have a bad block, we do not erase bad
blocks! */
4287 if (nand_block_checkbad(chip, ((loff_t) page) <<
4288 chip->page_shift, allowbbt)) {
4289 pr_warn("%s: attempt to erase a bad block at
page 0x%08x\n",
4290 __func__, page);
4291 ret = -EIO;
4292 goto erase_exit;
4293 }
4294
4295 /*
4296 * Invalidate the page cache, if we erase the block
which
4297 * contains the current cached page.
4298 */
4299 if (page <= chip->pagecache.pa
[PATCH v6 3/3] mtd: rawnand: Add support for secure regions in NAND memory
On a typical end product, a vendor may choose to secure some regions in
the NAND memory which are supposed to stay intact between FW upgrades.
The access to those regions will be blocked by a secure element like
Trustzone. So the normal world software like Linux kernel should not
touch these regions (including reading).
The regions are declared using a NAND chip DT property,
"secure-regions". So let's make use of this property in the raw NAND
core and skip access to the secure regions present in a system.
Signed-off-by: Manivannan Sadhasivam
---
drivers/mtd/nand/raw/nand_base.c | 111 +++
include/linux/mtd/rawnand.h | 4 ++
2 files changed, 115 insertions(+)
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index c33fa1b1847f..d2958549ba46 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -278,11 +278,47 @@ static int nand_block_bad(struct nand_chip *chip, loff_t
ofs)
return 0;
}
+/**
+ * nand_check_secure_region() - Check if the region is secured
+ * @chip: NAND chip object
+ * @offset: Offset of the region to check
+ * @size: Size of the region to check
+ *
+ * Checks if the region is secured by comparing the offset and size with the
+ * list of secure regions obtained from DT. Returns -EIO if the region is
+ * secured else 0.
+ */
+static int nand_check_secure_region(struct nand_chip *chip, loff_t offset, u32
size)
+{
+ int i, j;
+
+ /* Skip touching the secure regions if present */
+ for (i = 0, j = 0; i < chip->nr_secure_regions; i++, j += 2) {
+ /* First compare the start offset */
+ if (offset >= chip->secure_regions[j] &&
+ (offset < chip->secure_regions[j] + chip->secure_regions[j
+ 1]))
+ return -EIO;
+ /* ...then offset + size */
+ else if (offset < chip->secure_regions[i] &&
+(offset + size) >= chip->secure_regions[i])
+ return -EIO;
+ }
+
+ return 0;
+}
+
static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs)
{
+ int ret;
+
if (chip->options & NAND_NO_BBM_QUIRK)
return 0;
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, ofs, 0);
+ if (ret)
+ return ret;
+
if (chip->legacy.block_bad)
return chip->legacy.block_bad(chip, ofs);
@@ -397,6 +433,11 @@ static int nand_do_write_oob(struct nand_chip *chip,
loff_t to,
return -EINVAL;
}
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, to, ops->ooblen);
+ if (ret)
+ return ret;
+
chipnr = (int)(to >> chip->chip_shift);
/*
@@ -565,6 +606,11 @@ static int nand_block_isreserved(struct mtd_info *mtd,
loff_t ofs)
if (!chip->bbt)
return 0;
+
+ /* Check if the region is secured */
+ if (nand_check_secure_region(chip, ofs, 0))
+ return -EIO;
+
/* Return info from the table */
return nand_isreserved_bbt(chip, ofs);
}
@@ -2737,6 +2783,11 @@ static int nand_read_page_swecc(struct nand_chip *chip,
uint8_t *buf,
uint8_t *ecc_code = chip->ecc.code_buf;
unsigned int max_bitflips = 0;
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, ((loff_t)page <<
chip->page_shift), 0);
+ if (ret)
+ return ret;
+
chip->ecc.read_page_raw(chip, buf, 1, page);
for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
@@ -3127,6 +3178,11 @@ static int nand_do_read_ops(struct nand_chip *chip,
loff_t from,
int retry_mode = 0;
bool ecc_fail = false;
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, from, readlen);
+ if (ret)
+ return ret;
+
chipnr = (int)(from >> chip->chip_shift);
nand_select_target(chip, chipnr);
@@ -3458,6 +3514,11 @@ static int nand_do_read_oob(struct nand_chip *chip,
loff_t from,
pr_debug("%s: from = 0x%08Lx, len = %i\n",
__func__, (unsigned long long)from, readlen);
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, from, readlen);
+ if (ret)
+ return ret;
+
stats = mtd->ecc_stats;
len = mtd_oobavail(mtd, ops);
@@ -3709,6 +3770,11 @@ static int nand_write_page_swecc(struct nand_chip *chip,
const uint8_t *buf,
uint8_t *ecc_calc = chip->ecc.calc_buf;
const uint8_t *p = buf;
+ /* Check if the region is secured */
+ ret = nand_check_secure_region(chip, ((loff_t)page <<
chip->page_shift), 0);
+ if (ret)
+ return ret;
+
/* Software ECC calculation */
for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
