[patch 077/232] sdio: fix read buffer overflow

[patch 077/232] sdio: fix read buffer overflow

[akpm]

From: Roel Kluin <roel.kluin@gmail.com>

Avoid buffer underrun when parsing an invalid CISTPL_VERS_1.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
Cc: David Vrabel <david.vrabel@csr.com>
Cc: <linux-mmc@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/mmc/core/sdio_cis.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN drivers/mmc/core/sdio_cis.c~sdio-fix-read-buffer-overflow drivers/mmc/core/sdio_cis.c
--- a/drivers/mmc/core/sdio_cis.c~sdio-fix-read-buffer-overflow
+++ a/drivers/mmc/core/sdio_cis.c
@@ -40,7 +40,7 @@ static int cistpl_vers_1(struct mmc_card
 			nr_strings++;
 	}
 
-	if (buf[i-1] != '\0') {
+	if (nr_strings < 4) {
 		printk(KERN_WARNING "SDIO: ignoring broken CISTPL_VERS_1\n");
 		return 0;
 	}
_

Re: [patch 077/232] sdio: fix read buffer overflow

[Jonathan Cameron]

Hi All,

This patch is causing a regression with libertas 8686.
It's only finding 3 strings which I'm guessing means
it is an invalid CISTPL_VERS_1. Unfortunately the libertas_sdio
code relies on a string in one of them to tell it what model of
card we have.

Can someone confirm what the CIS_VERS_1 spec actually is?
I've found one vague reference to entries 3 and 4 being optional
but the simplified sdio spec refers to the pcmcia 3.2.10 spec
which I don't have easy access to.

Any suggestions on a work around?

Thanks,

Jonathan


> From: Roel Kluin <roel.kluin@gmail.com>
> 
> Avoid buffer underrun when parsing an invalid CISTPL_VERS_1.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> Cc: David Vrabel <david.vrabel@csr.com>
> Cc: <linux-mmc@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/mmc/core/sdio_cis.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -puN drivers/mmc/core/sdio_cis.c~sdio-fix-read-buffer-overflow drivers/mmc/core/sdio_cis.c
> --- a/drivers/mmc/core/sdio_cis.c~sdio-fix-read-buffer-overflow
> +++ a/drivers/mmc/core/sdio_cis.c
> @@ -40,7 +40,7 @@ static int cistpl_vers_1(struct mmc_card
>  			nr_strings++;
>  	}
>  
> -	if (buf[i-1] != '\0') {
> +	if (nr_strings < 4) {
>  		printk(KERN_WARNING "SDIO: ignoring broken CISTPL_VERS_1\n");
>  		return 0;
>  	}
> _
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch 077/232] sdio: fix read buffer overflow

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

Jonathan Cameron wrote:
> Hi All,
> 
> This patch is causing a regression with libertas 8686.
> It's only finding 3 strings which I'm guessing means
> it is an invalid CISTPL_VERS_1. Unfortunately the libertas_sdio
> code relies on a string in one of them to tell it what model of
> card we have.
> 
> Can someone confirm what the CIS_VERS_1 spec actually is?
> I've found one vague reference to entries 3 and 4 being optional
> but the simplified sdio spec refers to the pcmcia 3.2.10 spec
> which I don't have easy access to.

It's harmless if the tuple contains fewer so I think we should just try
and parse as many strings as possible.  Does this patch fix your regression?

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

[-- Attachment #2: 0001-mmc-sdio-don-t-require-CISTPL_VERS_1-to-contain-4-st.patch --]
[-- Type: text/x-diff, Size: 1319 bytes --]

>From 6ab16f09790e98832ee0077d67f8663bcf2b0ad1 Mon Sep 17 00:00:00 2001
From: David Vrabel <david.vrabel@csr.com>
Date: Thu, 1 Oct 2009 11:56:25 +0100
Subject: [PATCH] mmc: sdio: don't require CISTPL_VERS_1 to contain 4 strings

The PC Card 8.0 specification (vol. 4, section 3.2.10) says the
TPLLV1_INFO field of the CISTPL_VERS_1 tuple must contain 4 strings.
Some cards don't have all 4 so just parse as many as we can.

Signed-off-by: David Vrabel <david.vrabel@csr.com>
---
 drivers/mmc/core/sdio_cis.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index 6636354..b2dc4a7 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -29,6 +29,8 @@ static int cistpl_vers_1(struct mmc_card *card, struct sdio_func *func,
 	unsigned i, nr_strings;
 	char **buffer, *string;
 
+	/* Find all null-terminated (including zero length) strings in
+	   the TPLLVL1_INFO field. Trailing garbage is ignored. */
 	buf += 2;
 	size -= 2;
 
@@ -39,9 +41,7 @@ static int cistpl_vers_1(struct mmc_card *card, struct sdio_func *func,
 		if (buf[i] == 0)
 			nr_strings++;
 	}
-
-	if (nr_strings < 4) {
-		printk(KERN_WARNING "SDIO: ignoring broken CISTPL_VERS_1\n");
+	if (nr_strings == 0) {
 		return 0;
 	}
 
-- 
1.6.3.3

Re: [patch 077/232] sdio: fix read buffer overflow

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

David Vrabel wrote:
> 
> It's harmless if the tuple contains fewer so I think we should just try
> and parse as many strings as possible.  Does this patch fix your regression?

I spelt the field name wrong in the comment.  Use this patch instead,
please.

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

[-- Attachment #2: 0001-mmc-sdio-don-t-require-CISTPL_VERS_1-to-contain-4-st.patch --]
[-- Type: text/x-diff, Size: 1318 bytes --]

>From 41dc65a6198d2852a586ec0032fd5e03038fa32f Mon Sep 17 00:00:00 2001
From: David Vrabel <david.vrabel@csr.com>
Date: Thu, 1 Oct 2009 11:56:25 +0100
Subject: [PATCH] mmc: sdio: don't require CISTPL_VERS_1 to contain 4 strings

The PC Card 8.0 specification (vol. 4, section 3.2.10) says the
TPLLV1_INFO field of the CISTPL_VERS_1 tuple must contain 4 strings.
Some cards don't have all 4 so just parse as many as we can.

Signed-off-by: David Vrabel <david.vrabel@csr.com>
---
 drivers/mmc/core/sdio_cis.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index 6636354..e5ab44d 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -29,6 +29,8 @@ static int cistpl_vers_1(struct mmc_card *card, struct sdio_func *func,
 	unsigned i, nr_strings;
 	char **buffer, *string;
 
+	/* Find all null-terminated (including zero length) strings in
+	   the TPLLV1_INFO field. Trailing garbage is ignored. */
 	buf += 2;
 	size -= 2;
 
@@ -39,9 +41,7 @@ static int cistpl_vers_1(struct mmc_card *card, struct sdio_func *func,
 		if (buf[i] == 0)
 			nr_strings++;
 	}
-
-	if (nr_strings < 4) {
-		printk(KERN_WARNING "SDIO: ignoring broken CISTPL_VERS_1\n");
+	if (nr_strings == 0) {
 		return 0;
 	}
 
-- 
1.6.3.3

Re: [patch 077/232] sdio: fix read buffer overflow

[Jonathan Cameron]

David Vrabel wrote:
> David Vrabel wrote:
>> It's harmless if the tuple contains fewer so I think we should just try
>> and parse as many strings as possible.  Does this patch fix your regression?
> 
> I spelt the field name wrong in the comment.  Use this patch instead,
> please.
Works for me, so Tested-by: Jonathan Cameron <jic23@cam.ac.uk>

I guess it's just a question of posting it as a normal patch and seeing if
anyone screams.

Thanks,

Jonathan

RE: [patch 077/232] sdio: fix read buffer overflow

[Bing Zhao]

Hi,

> -----Original Message-----
> From: libertas-dev-bounces@lists.infradead.org [mailto:libertas-dev-bounces@lists.infradead.org] On
> Behalf Of Jonathan Cameron
> Sent: Thursday, October 01, 2009 7:12 AM
> To: David Vrabel
> Cc: linux-mmc@vger.kernel.org; akpm@linux-foundation.org; roel.kluin@gmail.com; libertas-
> dev@lists.infradead.org
> Subject: Re: [patch 077/232] sdio: fix read buffer overflow
> 
> David Vrabel wrote:
> > David Vrabel wrote:
> >> It's harmless if the tuple contains fewer so I think we should just try
> >> and parse as many strings as possible.  Does this patch fix your regression?
> >
> > I spelt the field name wrong in the comment.  Use this patch instead,
> > please.
> Works for me, so Tested-by: Jonathan Cameron <jic23@cam.ac.uk>
> 
> I guess it's just a question of posting it as a normal patch and seeing if
> anyone screams.

This patch works for me too.

Tested-by: Bing Zhao <bzhao@marvell.com>

Thanks,

Bing

> 
> Thanks,
> 
> Jonathan
> 
> _______________________________________________
> libertas-dev mailing list
> libertas-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev