| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAARK3HkRgchmPZRpHfHj69OMUn=K7Zp-az5YYtBOW_jdASgYyw@mail.gmail.com>
Date: Fri, 9 Aug 2019 19:53:45 +0530
From: Sagar Kadam <sagar.kadam@...ive.com>
To: Vignesh Raghavendra <vigneshr@...com>
Cc: marek.vasut@...il.com, tudor.ambarus@...rochip.com,
dwmw2@...radead.org, computersforpeace@...il.com,
miquel.raynal@...tlin.com, richard@....at,
Palmer Dabbelt <palmer@...ive.com>,
Albert Ou <aou@...s.berkeley.edu>,
Paul Walmsley <paul.walmsley@...ive.com>,
linux-mtd@...ts.infradead.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v7 4/4] mtd: spi-nor: add locking support for is25wp256 device
Hello Vignesh,
On Fri, Aug 9, 2019 at 4:57 PM Vignesh Raghavendra <vigneshr@...com> wrote:
>
> Hi Sagar,
>
> On 03/07/19 12:09 AM, Sagar Shrikant Kadam wrote:
> [...]> +static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > +{
> > + int status_old, status_new, blk_prot;
> > + u8 mask;
> > + u8 shift;
> > + u8 pow, ret, func_reg;
> > + bool use_top;
> > + loff_t lock_len;
> > +
> > + if (nor->flags & SNOR_F_HAS_BP3)
> > + mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
> > + else
> > + mask = SR_BP2 | SR_BP1 | SR_BP0;
> > +
> > + shift = ffs(mask) - 1;
> > +
> > + status_old = read_sr(nor);
> > +
> > + /* if status reg is Write protected don't update bit protection */
> > + if (status_old & SR_SRWD) {
> > + dev_err(nor->dev,
> > + "SR is write protected, can't update BP bits...\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = spi_nor_select_zone(nor, ofs, len, status_old, &use_top, 1);
> > + if (!ret)
> > + /* Older protected blocks include the new requested block's */
> > + return 0;
> > + else if (ret < 0)
> > + return ret;
> > +
> > + func_reg = spi_nor_read_fr(nor);
>
Thank you for reviewing the patch.
> Sorry, I don't understand where func_reg is used? Since TBS OTP, how
> does issi_lock() code comprehend that TBS bit value in OTP register and
> use_top returned by spi_nor_select_zone() are matching before we go
> ahead and program status register.
>
In my earlier version of this patch, you had suggested an approach to skip
updating the TBS OTP bits as ones written that cannot be changed.
Actually, func_reg was used to update the OTP section, sorry I missed
removing the func_reg reference here as per your suggestion.
I will update a new version of this patch which will not modify the OTP
region and will return with an error when locking the requested region by
the user is not possible.
> We should reject locking request if top-bottom calculation does not
> match OTP bit. Where is that done?
>
> Regards
> Vignesh
>
Thanks & BR,
Sagar
> > + /* lock_len: length of region that should end up locked */
> > + if (use_top)
> > + lock_len = nor->mtd.size - ofs;
> > + else
> > + lock_len = ofs + len;
> > +
> > + pow = order_base_2(lock_len);
> > + blk_prot = mask & (((pow + 1) & 0xf) << shift);
> > + if (lock_len <= 0) {
> > + dev_err(nor->dev, "invalid Length to protect");
> > + return -EINVAL;
> > + }
> > +
> > + status_new = status_old | blk_prot;
> > + if (status_old == status_new)
> > + return 0;
> > +
> > + return write_sr_and_check(nor, status_new, mask);
> > +}
> > +
> > +/**
> > * issi_unlock() - clear BP[0123] write-protection.
> > * @nor: pointer to a 'struct spi_nor'.
> > * @ofs: offset from which to unlock memory.
> > @@ -4171,6 +4338,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> > if (JEDEC_MFR(info) == SNOR_MFR_ISSI &&
> > info->flags & SPI_NOR_HAS_LOCK &&
> > info->flags & SPI_NOR_HAS_BP3) {
> > + nor->flash_lock = issi_lock;
> > nor->flash_unlock = issi_unlock;
> > }
> >
> > @@ -4194,6 +4362,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> > nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> > if (info->flags & USE_CLSR)
> > nor->flags |= SNOR_F_USE_CLSR;
> > + if (info->flags & SPI_NOR_HAS_BP3)
> > + nor->flags |= SNOR_F_HAS_BP3;
> >
> > if (info->flags & SPI_NOR_NO_ERASE)
> > mtd->flags |= MTD_NO_ERASE;
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index f6fa70f..26dbf48 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -40,6 +40,8 @@
> > #define SPINOR_OP_RDSR 0x05 /* Read status register */
> > #define SPINOR_OP_WRSR 0x01 /* Write status register 1 byte */
> > #define SPINOR_OP_RDSR2 0x3f /* Read status register 2 */
> > +#define SPINOR_OP_RDFR 0x48 /* Read Function register */
> > +#define SPINOR_OP_WRFR 0x42 /* Write Function register 1 byte */
> > #define SPINOR_OP_WRSR2 0x3e /* Write status register 2 */
> > #define SPINOR_OP_READ 0x03 /* Read data bytes (low frequency) */
> > #define SPINOR_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */
> > @@ -139,6 +141,9 @@
> > /* Enhanced Volatile Configuration Register bits */
> > #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */
> >
> > +/*Function register bit */
> > +#define FR_TB BIT(1) /*ISSI: Top/Bottom protect */
> > +
> > /* Flag Status Register bits */
> > #define FSR_READY BIT(7) /* Device status, 0 = Busy, 1 = Ready */
> > #define FSR_E_ERR BIT(5) /* Erase operation status */
> >
>
> --
> Regards
> Vignesh
Powered by blists - more mailing lists