[PATCH] mtd: sa1100: avoid VLA in sa1100_setup_mtd

[PATCH] mtd: sa1100: avoid VLA in sa1100_setup_mtd

[Arnd Bergmann]

Enabling -Wvla found another variable-length array with randconfig
testing:

drivers/mtd/maps/sa1100-flash.c: In function 'sa1100_setup_mtd':
drivers/mtd/maps/sa1100-flash.c:224:10: error: ISO C90 forbids variable length array 'cdev' [-Werror=vla]

As far as I can tell, there is an upper bound on the number of resources
that can be passed, based on the number of CS lines on the bus.
In practice, all boards we support have either one or two resources,
but using six to be on the safe side has no extra cost.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/mtd/maps/sa1100-flash.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/maps/sa1100-flash.c b/drivers/mtd/maps/sa1100-flash.c
index 784c6e1a0391..234573b401bd 100644
--- a/drivers/mtd/maps/sa1100-flash.c
+++ b/drivers/mtd/maps/sa1100-flash.c
@@ -23,6 +23,8 @@
 #include <asm/sizes.h>
 #include <asm/mach/flash.h>
 
+#define SA1100_NUM_CS 6
+
 struct sa_subdev_info {
 	char name[16];
 	struct map_info map;
@@ -157,7 +159,7 @@ static struct sa_info *sa1100_setup_mtd(struct platform_device *pdev,
 	/*
 	 * Count number of devices.
 	 */
-	for (nr = 0; ; nr++)
+	for (nr = 0; nr < SA1100_NUM_CS; nr++)
 		if (!platform_get_resource(pdev, IORESOURCE_MEM, nr))
 			break;
 
@@ -221,7 +223,7 @@ static struct sa_info *sa1100_setup_mtd(struct platform_device *pdev,
 		info->mtd = info->subdev[0].mtd;
 		ret = 0;
 	} else if (info->num_subdev > 1) {
-		struct mtd_info *cdev[nr];
+		struct mtd_info *cdev[SA1100_NUM_CS];
 		/*
 		 * We detected multiple devices.  Concatenate them together.
 		 */
-- 
2.18.0

Re: [PATCH] mtd: sa1100: avoid VLA in sa1100_setup_mtd

[Boris Brezillon]

Hi Arnd,

On Wed, 10 Oct 2018 20:44:50 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> Enabling -Wvla found another variable-length array with randconfig
> testing:
> 
> drivers/mtd/maps/sa1100-flash.c: In function 'sa1100_setup_mtd':
> drivers/mtd/maps/sa1100-flash.c:224:10: error: ISO C90 forbids variable length array 'cdev' [-Werror=vla]
> 
> As far as I can tell, there is an upper bound on the number of resources
> that can be passed, based on the number of CS lines on the bus.
> In practice, all boards we support have either one or two resources,
> but using six to be on the safe side has no extra cost.

Why not dynamically allocate cdev instead? That removes any kind of
guessing on the max value, and it shouldn't hurt much since this code is
in the probe path.

--->8---
diff --git a/drivers/mtd/maps/sa1100-flash.c b/drivers/mtd/maps/sa1100-flash.c
index 784c6e1a0391..fd5fe12d7461 100644
--- a/drivers/mtd/maps/sa1100-flash.c
+++ b/drivers/mtd/maps/sa1100-flash.c
@@ -221,7 +221,14 @@ static struct sa_info *sa1100_setup_mtd(struct platform_device *pdev,
                info->mtd = info->subdev[0].mtd;
                ret = 0;
        } else if (info->num_subdev > 1) {
-               struct mtd_info *cdev[nr];
+               struct mtd_info **cdev;
+
+               cdev = kmalloc_array(nr, sizeof(*cdev), GFP_KERNEL);
+               if (!cdev) {
+                       ret = -ENOMEM;
+                       goto err;
+               }
+
                /*
                 * We detected multiple devices.  Concatenate them together.
                 */
@@ -230,6 +237,7 @@ static struct sa_info *sa1100_setup_mtd(struct platform_device *pdev,
 
                info->mtd = mtd_concat_create(cdev, info->num_subdev,
                                              plat->name);
+               kfree(cdev);
                if (info->mtd == NULL) {
                        ret = -ENXIO;
                        goto err;

Re: [PATCH] mtd: sa1100: avoid VLA in sa1100_setup_mtd

[Arnd Bergmann]

On Fri, Oct 12, 2018 at 11:16 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> Hi Arnd,
>
> On Wed, 10 Oct 2018 20:44:50 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
>
> > Enabling -Wvla found another variable-length array with randconfig
> > testing:
> >
> > drivers/mtd/maps/sa1100-flash.c: In function 'sa1100_setup_mtd':
> > drivers/mtd/maps/sa1100-flash.c:224:10: error: ISO C90 forbids variable length array 'cdev' [-Werror=vla]
> >
> > As far as I can tell, there is an upper bound on the number of resources
> > that can be passed, based on the number of CS lines on the bus.
> > In practice, all boards we support have either one or two resources,
> > but using six to be on the safe side has no extra cost.
>
> Why not dynamically allocate cdev instead? That removes any kind of
> guessing on the max value, and it shouldn't hurt much since this code is
> in the probe path.

Fine with me as well, If you prefer that one, please just add
Reported-by: Arnd Bergmann <arnd@arndb.de>

       Arnd

Re: [PATCH] mtd: sa1100: avoid VLA in sa1100_setup_mtd

[Boris Brezillon]

On Fri, 12 Oct 2018 11:19:52 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Fri, Oct 12, 2018 at 11:16 AM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > Hi Arnd,
> >
> > On Wed, 10 Oct 2018 20:44:50 +0200
> > Arnd Bergmann <arnd@arndb.de> wrote:
> >  
> > > Enabling -Wvla found another variable-length array with randconfig
> > > testing:
> > >
> > > drivers/mtd/maps/sa1100-flash.c: In function 'sa1100_setup_mtd':
> > > drivers/mtd/maps/sa1100-flash.c:224:10: error: ISO C90 forbids variable length array 'cdev' [-Werror=vla]
> > >
> > > As far as I can tell, there is an upper bound on the number of resources
> > > that can be passed, based on the number of CS lines on the bus.
> > > In practice, all boards we support have either one or two resources,
> > > but using six to be on the safe side has no extra cost.  
> >
> > Why not dynamically allocate cdev instead? That removes any kind of
> > guessing on the max value, and it shouldn't hurt much since this code is
> > in the probe path.  
> 
> Fine with me as well, If you prefer that one, please just add
> Reported-by: Arnd Bergmann <arnd@arndb.de>

Oh, I thought I'd let you send a v2, but I can do it if you prefer.

Re: [PATCH] mtd: sa1100: avoid VLA in sa1100_setup_mtd

[Kees Cook]

On Fri, Oct 12, 2018 at 2:22 AM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Fri, 12 Oct 2018 11:19:52 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
>
>> On Fri, Oct 12, 2018 at 11:16 AM Boris Brezillon
>> <boris.brezillon@bootlin.com> wrote:
>> >
>> > Hi Arnd,
>> >
>> > On Wed, 10 Oct 2018 20:44:50 +0200
>> > Arnd Bergmann <arnd@arndb.de> wrote:
>> >
>> > > Enabling -Wvla found another variable-length array with randconfig
>> > > testing:
>> > >
>> > > drivers/mtd/maps/sa1100-flash.c: In function 'sa1100_setup_mtd':
>> > > drivers/mtd/maps/sa1100-flash.c:224:10: error: ISO C90 forbids variable length array 'cdev' [-Werror=vla]
>> > >
>> > > As far as I can tell, there is an upper bound on the number of resources
>> > > that can be passed, based on the number of CS lines on the bus.
>> > > In practice, all boards we support have either one or two resources,
>> > > but using six to be on the safe side has no extra cost.
>> >
>> > Why not dynamically allocate cdev instead? That removes any kind of
>> > guessing on the max value, and it shouldn't hurt much since this code is
>> > in the probe path.
>>
>> Fine with me as well, If you prefer that one, please just add
>> Reported-by: Arnd Bergmann <arnd@arndb.de>
>
> Oh, I thought I'd let you send a v2, but I can do it if you prefer.

Olof just pointed out to me that neither fix landed for this? What's
needed for this?

Thanks!

-- 
Kees Cook

Re: [PATCH] mtd: sa1100: avoid VLA in sa1100_setup_mtd

[Boris Brezillon]

Hi Kees,

On Sun, 28 Oct 2018 19:13:26 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Fri, Oct 12, 2018 at 2:22 AM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Fri, 12 Oct 2018 11:19:52 +0200
> > Arnd Bergmann <arnd@arndb.de> wrote:
> >  
> >> On Fri, Oct 12, 2018 at 11:16 AM Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:  
> >> >
> >> > Hi Arnd,
> >> >
> >> > On Wed, 10 Oct 2018 20:44:50 +0200
> >> > Arnd Bergmann <arnd@arndb.de> wrote:
> >> >  
> >> > > Enabling -Wvla found another variable-length array with randconfig
> >> > > testing:
> >> > >
> >> > > drivers/mtd/maps/sa1100-flash.c: In function 'sa1100_setup_mtd':
> >> > > drivers/mtd/maps/sa1100-flash.c:224:10: error: ISO C90 forbids variable length array 'cdev' [-Werror=vla]
> >> > >
> >> > > As far as I can tell, there is an upper bound on the number of resources
> >> > > that can be passed, based on the number of CS lines on the bus.
> >> > > In practice, all boards we support have either one or two resources,
> >> > > but using six to be on the safe side has no extra cost.  
> >> >
> >> > Why not dynamically allocate cdev instead? That removes any kind of
> >> > guessing on the max value, and it shouldn't hurt much since this code is
> >> > in the probe path.  
> >>
> >> Fine with me as well, If you prefer that one, please just add
> >> Reported-by: Arnd Bergmann <arnd@arndb.de>  
> >
> > Oh, I thought I'd let you send a v2, but I can do it if you prefer.  
> 
> Olof just pointed out to me that neither fix landed for this? What's
> needed for this?

Nothing in particular, I was planning on sending a new version after
-rc1 is out and then queue it for 4.21 (5.1?) (this patch came in a bit
late, and I had already stopped taking patches for 4.20).

If you consider this a fix or want to have it in 4.20 for other reasons,
just let me know and I'll queue it to the -fixes branch.

Regards,

Boris

Re: [PATCH] mtd: sa1100: avoid VLA in sa1100_setup_mtd

[Arnd Bergmann]

On Mon, Oct 29, 2018 at 8:30 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Sun, 28 Oct 2018 19:13:26 -0700 Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Oct 12, 2018 at 2:22 AM, Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > > On Fri, 12 Oct 2018 11:19:52 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Fri, Oct 12, 2018 at 11:16 AM Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > > Oh, I thought I'd let you send a v2, but I can do it if you prefer.
> >
> > Olof just pointed out to me that neither fix landed for this? What's
> > needed for this?
>
> Nothing in particular, I was planning on sending a new version after
> -rc1 is out and then queue it for 4.21 (5.1?) (this patch came in a bit
> late, and I had already stopped taking patches for 4.20).
>
> If you consider this a fix or want to have it in 4.20 for other reasons,
> just let me know and I'll queue it to the -fixes branch.

We generally try to have a kernel that can be built in any configuration
without warnings, so please add it for v4.20.

        Arnd