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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 06 Mar 2014 09:57:52 +0100 From: Christian Riesch <christian.riesch@...cron.at> To: Brian Norris <computersforpeace@...il.com> cc: Amul Kumar Saha <amul.saha@...sung.com>, Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>, Kyungmin Park <kyungmin.park@...sung.com>, linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v2 RESEND 2/2] mtd: Fix the behavior of otp write if there is not enough room for data Hi Brian, --On March 06, 2014 00:49 -0800 Brian Norris <computersforpeace@...il.com> wrote: > Hi, > > On Wed, Mar 05, 2014 at 09:50:35AM +0100, Christian Riesch wrote: >> On March 04, 2014 23:20 -0800 Brian Norris <computersforpeace@...il.com> >> wrote: >> > On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote: >> >> An OTP write shall write as much data as possible to the OTP memory >> >> and return the number of bytes that have actually been written. >> >> If no data could be written at all due to lack of OTP memory, >> >> return -ENOSPC. >> >> >> >> Signed-off-by: Christian Riesch <christian.riesch@...cron.at> >> >> Cc: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com> >> >> Cc: Kyungmin Park <kyungmin.park@...sung.com> >> >> Cc: Amul Kumar Saha <amul.saha@...sung.com> >> >> --- >> >> drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++++++++++-- >> >> drivers/mtd/devices/mtd_dataflash.c | 13 +++++-------- >> >> drivers/mtd/mtdchar.c | 7 +++++++ >> >> drivers/mtd/onenand/onenand_base.c | 10 +++++++++- >> >> 4 files changed, 32 insertions(+), 11 deletions(-) >> >> >> >> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c >> >> b/drivers/mtd/chips/cfi_cmdset_0001.c index 7aa581f..cf423a6 100644 >> >> --- a/drivers/mtd/chips/cfi_cmdset_0001.c >> >> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c >> >> @@ -2387,8 +2387,17 @@ static int >> >> cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from, >> >> size_t len, size_t *retlen, >> >> u_char *buf) >> >> { >> >> - return cfi_intelext_otp_walk(mtd, from, len, retlen, >> >> - buf, do_otp_write, 1); >> >> + int ret; >> >> + >> >> + ret = cfi_intelext_otp_walk(mtd, from, len, retlen, >> >> + buf, do_otp_write, 1); >> >> + >> >> + /* if no data could be written due to lack of OTP memory, >> >> + return ENOSPC */ >> > >> > /* >> > * Can you use this style of mult-line comments please? >> > * It's in Documentation/CodingStyle >> > */ >> > >> >> Ok, I will change that. >> >> >> + if (!ret && len && !(*retlen)) >> >> + return -ENOSPC; >> > >> > Couldn't (shouldn't) this check be pushed to the common >> > mtd_write_user_prot_reg() helper in mtdcore.c? >> >> Yes, I don't see why this wouldn't work. But I thought the code >> would be easier to understand if we return the correct error code as >> soon as the error is detected, not using some additional logic in >> some other function. What do you think? > > No, this is the purpose of the mtd_xxx() wrappers for the mtd->_xxx > implementations. That way we don't have to inconsistently implement the > same checks in every driver. This caught a few bugs for > mtd_{read,write}() when we unified the bounds checking, I think. Ok, I will change that. >> > And once you do that, you >> > will see that cfi_intelext_write_user_prot_reg() (and other >> > mtd->_write_user_prot_reg() implementations) will never be called with >> > len == 0. So this just becomes (in mtdcore.c): >> > >> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c >> > index 0a7d77e65335..ee6730748f7e 100644 >> > --- a/drivers/mtd/mtdcore.c >> > +++ b/drivers/mtd/mtdcore.c >> > @@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg); >> > int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t >> > *retlen, struct otp_info *buf) >> > { >> > + int ret; >> > + >> > if (!mtd->_get_user_prot_info) >> > return -EOPNOTSUPP; >> > if (!len) >> > return 0; >> > - return mtd->_get_user_prot_info(mtd, len, retlen, buf); >> > + ret = mtd->_get_user_prot_info(mtd, len, retlen, buf); >> > + if (ret) >> > + return ret; >> > + return !(*retlen) ? -ENOSPC: 0; >> > } >> > EXPORT_SYMBOL_GPL(mtd_get_user_prot_info); >> > >> > >> >> + >> >> + return ret; >> >> } >> >> >> >> static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd, >> >> diff --git a/drivers/mtd/devices/mtd_dataflash.c >> >> b/drivers/mtd/devices/mtd_dataflash.c index 09c69ce..5236d85 100644 >> >> --- a/drivers/mtd/devices/mtd_dataflash.c >> >> +++ b/drivers/mtd/devices/mtd_dataflash.c >> >> @@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct >> >> mtd_info *mtd, struct dataflash *priv = mtd->priv; >> >> int status; >> >> >> > >> > I'm not sure I quite follow the logic for the following hunk. I think >> > it deserves some more explanation, either in your commit or in a >> > comment. As it stands, you're deleting a comment and potentially >> > changing the return code behavior subtly. >> > >> >> - if (len > 64) >> >> - return -EINVAL; >> >> - >> >> - /* Strictly speaking, we *could* truncate the write ... but >> >> - * let's not do that for the only write that's ever possible. >> >> - */ >> >> - if ((from + len) > 64) >> >> - return -EINVAL; >> >> + if ((from + len) > 64) { >> >> + len = 64 - from; >> > >> > Why are you reassigning len? Are you trying to undo the comment above, >> > so that you *can* truncate the write? (It looks like there are other >> > implmentations which will truncate the write and return -ENOSPC, FWIW.) >> >> Currently we have two kind of implementations: We have >> implementations like this one which will refuse to write any data if >> the write requests more data to be written than space is available. >> And we have implementations like cfi_intelext_write_user_prot_reg >> that will truncate the write and write as much data that is possible >> (and return the number of bytes that actually have been written, >> -ENOSPC shall only be returned if no data could be written at all). >> >> For a harmonization one of the implementations and their behavior >> must be changed. I chose to change it to "write as much as >> possible/truncate the write" since this is how a write should behave >> (http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html). >> And yes, this is why I try to undo the comment. > > OK, that makes sense. Then I think you should add a comment here in > dataflash_write_user_otp() to say that you *are* truncating (or possibly > rearrange the logic?), as it's not 100% clear what you're trying to do > here. And add a small blurb to note this in the commit description. Some > version of the above two paragraphs would make a nice > addition/replacement to the patch description. Ok, I will add a comment. >> But if you are afraid that this will break things for current users >> of the functions, I would keep the old behavior. What do you think? > > No, I believe your semantics make sense, and it's not a major breakage. > Ok. >> > >> >> + if (len <= 0) >> >> + return -ENOSPC; >> >> + } >> >> >> >> /* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes >> >> * IN: ignore all >> >> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c >> >> index 0edb0ca..db99031 100644 >> >> --- a/drivers/mtd/mtdchar.c >> >> +++ b/drivers/mtd/mtdchar.c >> >> @@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file, >> >> const char __user *buf, size_t c default: >> >> ret = mtd_write(mtd, *ppos, len, &retlen, kbuf); >> >> } >> >> + /* return -ENOSPC only if no data was written */ >> >> + if ((ret == -ENOSPC) && (total_retlen)) { >> >> + ret = 0; >> >> + retlen = 0; >> >> + /* drop the remaining data */ >> >> + count = 0; >> > >> > This block can just be a 'break' statement, no? >> >> No. mtdchar_write may split the write into several calls of >> mtd_write, mtd_write_user_prot_reg... It will call mtd_write, >> mtd_write_user_prot_reg as long as there is data to be written. If >> the write hits the boundary of the memory, the last call of >> mtd_write_user_prot_reg will return -ENOSPC. If this was the only >> call of mtd_write_user_prot_reg (so no data could be written at >> all), returning -ENOSPC to the user is fine. However, if data has >> been written before, we must not return -ENOSPC, we must return the >> number of bytes that have actually been written. >> >> So at least it must be >> >> if ((ret == -ENOSPC) && (total_retlen)) { >> ret = 0; > > ^^^ this line will have no effect, since 'ret' is not used outside the > while loop. > You are right, sorry for that. >> break; >> } >> >> Which one do you prefer? > > I prefer the following :) It's fewer lines and more straightforward, I > think. > > if ((ret == -ENOSPC) && total_retlen) > break; > Ok, I'll change that. Christian >> > >> >> + } >> > >> > I'm a bit wary of changing the behavior of non-OTP writes. At a >> > minimum, the patch description needs to acknowledge that this affects >> > more than just OTP writes. But after a cursory review of mtd->_write() >> > implementations, it looks like there's no driver which could be >> > returning -ENOSPC already, so this change is probably OK. >> >> The behavior of non-OTP writes is not changed at all. At the begin >> of mtdchar_write, a check against mtd->size is done, and the write >> is truncated. Therefore, non-OTP writes will never hit the end of >> memory in the write function. > > Right, and actually mtd_write() never gives -ENOSPC; it returns -EINVAL. > So this is fine. > > Brian > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists