linux-kernel - Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190613113756.53nzb6o2vuurep2a@laureti-dev>
Date:   Thu, 13 Jun 2019 13:37:57 +0200
From:   Helmut Grohne <helmut.grohne@...enta.de>
To:     Naga Sureshkumar Relli <nagasure@...inx.com>
CC:     "bbrezillon@...nel.org" <bbrezillon@...nel.org>,
        "miquel.raynal@...tlin.com" <miquel.raynal@...tlin.com>,
        "richard@....at" <richard@....at>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "computersforpeace@...il.com" <computersforpeace@...il.com>,
        "marek.vasut@...il.com" <marek.vasut@...il.com>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Michal Simek <michals@...inx.com>,
        "nagasureshkumarrelli@...il.com" <nagasureshkumarrelli@...il.com>
Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm
 pl353 smc nand interface

Hi Naga,

On Thu, Jun 13, 2019 at 10:18:00AM +0000, Naga Sureshkumar Relli wrote:
> I spent much of time to address all your comments.
> All are addressed and tested. except the above one(address offset calculation)
> I didn't see any issue with the address calculation.

Let me first point out that this comment was not trying to imply a bug.
I was trying to understand the code by comparing it to similar code and
that turned up an inconsistency, which can be intentional or a bug in
either of the sides being compared.

> for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> 	nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
> 			 (8 * i);
> }
> If you go through the nand_base.c, there nand_fill_column_cycles() API, fills the first two or one address cycle
> Based on bus width and page size.
> That means, addrs[0]/[1] will be updated here.

The problem at hand is that `addrs` is imprecise. In this code, there
are `instr->ctx.addr.addrs`, `addrs`, and `nfc_op->addrs`. All of them
are different. My original remark was targeting the possible confusion
of these different `addrs`.

> And the page is updated to the next offsets.
> In the similar way we have to extract the offsets in driver.
> So the first four address bytes are stored using the above for() loop and if the
> Address cycles are more than 4, then store the remaining offsets as well.
> 
> I just compared the offsets that are updated in driver with the offsets(page and column) that the frame work(nand_base.c) is sending, and the offsets are same.
> I have also checked these offsets with older driver(not exec_op() implemented) and both are matching.
> 
> So I didn't see any issue with this addrs calculation.
> As per the statement mentioned by you, this driver consumes addr[0], addr[1], addr[2], addr[3] and
> If more address cycles needed, then addr[4] and addr[5]. This is correct.

Again, the lack of precision makes it difficult to discuss the matter.
You refer to `addr`, but there is no `addr`. I assume that you meant
`addrs` here. Based on that assumption, your second last statement is
wrong. The driver consumes `addrs[0]|addrs[-offset]` rather than
`addrs[0]` as the first byte.  Then it proceeds consuming
`addrs[1-offset]` instead of `addrs[1]`, `addrs[2-offset]` instead of
`addrs[2]`, and `addrs[3-offset]` instead of `addrs[3]`. Finally it
consumes `addrs[4]` and `addrs[5]` if more cycles are needed.

I would not have commented the code if it were actually using `addrs[0]`
through `addrs[5]`. Your description looks reasonable to me, but it
doesn't match the code.

I'm looking forward to the next version of the patch.

Helmut