linux-kernel - [PATCH] mmc: toughen get_ro() and get

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20080605171013.GA10513@polina.dev.rtsoft.ru>
Date:	Thu, 5 Jun 2008 21:10:13 +0400
From:	Anton Vorontsov <avorontsov@...mvista.com>
To:	Marc Pignat <marc.pignat@...s.ch>
Cc:	Kumar Gala <galak@...nel.crashing.org>,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Pierre Ossman <drzeus-mmc@...eus.cx>,
	Jochen Friedrich <jochen@...am.de>,
	Timur Tabi <timur@...escale.com>, linuxppc-dev@...abs.org,
	linux-kernel@...r.kernel.org,
	spi-devel-general@...ts.sourceforge.net
Subject: [PATCH] mmc: toughen get_ro() and get_cd() return values

For the sake of safety, document that drivers should return only
1 or 0 from the get_ro() and get_cd() callbacks. Also document context
in which these callbacks should be executed.

wbsd driver modified to comply with this requirement.

Also, fix mmc_spi driver to not return raw values from the platform
get_cd hook (oops).

Suggested-by: Marc Pignat <marc.pignat@...s.ch>
Signed-off-by: Anton Vorontsov <avorontsov@...mvista.com>
---

On Thu, Jun 05, 2008 at 05:58:59PM +0200, Marc Pignat wrote:
[...]
> > >  * get_ro will return:
> > >  *   0 for a read/write card
> > >  *   1 for a read-only card 
> > 
> > This isn't always practical. For example, host is using u8 register for
> > the status, so it might safely return u8 & mask, that will always fit
> > into int. Or very smart/adventurous authors might be aware that, for the
> > particular host, mask's bit isn't last, and safely do uXX & mask. :-)
> > 
> > The above is weak argument of course, since it is about optimization.
> 
> Ack, we will gain at most 1-4 assembly instructions, in a function that
> is unlikely to be called more than once a second.
> 
> > 
> > As an counter-evidence, the strict scheme that you described probably
> > less error prone. But is it? To implement it we'll need something like
> > WARN_ON(ret > 0 && ret != 1) to catch erroneous users. Take a closer
> > look though, will it catch uXX & lastbit case? Nope. :-)
> 
> WARN_ON(ret > 0 && ret != 1 || ret == INT_MIN) will do ;)
> 
> I agree with you once more, I never thinked about a runtime check.
> 
> I don't really want to see a WARN_ON(foo) after each call to get_ro or get_cd.
> 
> But I'm sure if we specify "give me a positive value when a card is detected",
> someone will write gpio & bit, and three years later, someone will fall in
> the (gpio & lastbit < 0 case).
> 
> So we should specify: "give me 1 whan a card is present, 0 when not, -ENOSYS if
> you don't know and a negative errno when something else goes wrong".

Well, ok.

Pierre, I see you didn't yet pushed out the mmc tree, so.. would you
prefer this patch folded into 0/3 series and resent?

 drivers/mmc/host/mmc_spi.c |    2 +-
 drivers/mmc/host/wbsd.c    |    2 +-
 include/linux/mmc/host.h   |   16 ++++++++++++++--
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 85d9853..4e82f64 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1139,7 +1139,7 @@ static int mmc_spi_get_cd(struct mmc_host *mmc)
 	struct mmc_spi_host *host = mmc_priv(mmc);
 
 	if (host->pdata && host->pdata->get_cd)
-		return host->pdata->get_cd(mmc->parent);
+		return !!host->pdata->get_cd(mmc->parent);
 	return -ENOSYS;
 }
 
diff --git a/drivers/mmc/host/wbsd.c b/drivers/mmc/host/wbsd.c
index be624a0..9283b85 100644
--- a/drivers/mmc/host/wbsd.c
+++ b/drivers/mmc/host/wbsd.c
@@ -939,7 +939,7 @@ static int wbsd_get_ro(struct mmc_host *mmc)
 
 	spin_unlock_bh(&host->lock);
 
-	return csr & WBSD_WRPT;
+	return !!(csr & WBSD_WRPT);
 }
 
 static const struct mmc_host_ops wbsd_ops = {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ef3b773..753b723 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -56,8 +56,20 @@ struct mmc_host_ops {
 	 * since underlaying controller might implement them in an expensive
 	 * and/or slow way.
 	 *
-	 * .get_ro and .get_cd should return >= 0 for their logical values,
-	 * or negative errno value in case of error.
+	 * Also note that these functions might sleep, so don't call them
+	 * in the atomic contexts!
+	 *
+	 * Return values for the get_ro callback should be:
+	 *   0 for a read/write card
+	 *   1 for a read-only card
+	 *   -ENOSYS when not supported (equal to NULL callback)
+	 *   or a negative errno value when something bad happened
+	 *
+	 * Return values for the get_ro callback should be:
+	 *   0 for a absent card
+	 *   1 for a present card
+	 *   -ENOSYS when not supported (equal to NULL callback)
+	 *   or a negative errno value when something bad happened
 	 */
 	void	(*set_ios)(struct mmc_host *host, struct mmc_ios *ios);
 	int	(*get_ro)(struct mmc_host *host);
-- 
1.5.5.1

--
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/