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>] [day] [month] [year] [list]
Date:   Mon, 2 Oct 2023 14:12:04 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     dregan@...l.com
Cc:     bcm-kernel-feedback-list@...adcom.com,
        linux-mtd@...ts.infradead.org, f.fainelli@...il.com,
        rafal@...ecki.pl, joel.peshkin@...adcom.com,
        computersforpeace@...il.com, dan.beygelman@...adcom.com,
        william.zhang@...adcom.com, frieder.schrempf@...tron.de,
        linux-kernel@...r.kernel.org, vigneshr@...com, richard@....at,
        bbrezillon@...nel.org, kdasu.kdev@...il.com
Subject: Re: [PATCH] mtd: rawnand: brcmnand: Initial exec_op implementation

Hi David,

> > > +
> > > +	for (i = 0; i < subop->ninstrs; i++) {
> > > +		instr = &subop->instrs[i];
> > > +
> > > +		if ((instr->type == NAND_OP_CMD_INSTR) &&
> > > +			(instr->ctx.cmd.opcode == NAND_CMD_STATUS))
> > > +			status = 1;
> > > +		else if (status && (instr->type == NAND_OP_DATA_IN_INSTR)) {
> > > +			/*
> > > +			 * need to fake the nand device write protect because nand_base does a
> > > +			 * nand_check_wp which calls nand_status_op NAND_CMD_STATUS which checks
> > > +			 * that the nand is not write protected before an operation starts.
> > > +			 * The problem with this is it's done outside exec_op so the nand is
> > > +			 * write protected and this check will fail until the write or erase
> > > +			 * or write back operation actually happens where we turn off wp.
> > > +			 */  
> > 
> > If there is a problem with the core it needs to be handled in the core,
> > not workarounded here. The whole logic with the status property seems
> > really wrong.
> >   
> 
> I'm trying to change our current code functionality as little as
> possible by having this function in the same way as it always has
> and I do not want to make changes too much outside the scope
> of this exec_op change.

I understand, and this is probably the best first approach, but if
there is really an issue here with the behavior of the core (or one of
its helpers) we need to fix it properly rather than workarounding it.

Thanks,
Miquèl

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ