The kernel would like to have all stack VLA usage removed[1]. rsi uses
a VLA based on 'blksize'. Elsewhere in the SDIO code maximum block size
is defined using a magic number. We can use a pre-processor defined
constant and declare the array to maximum size. We add a check before
accessing the array in case of programmer error.
[1]: https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Tobin C. Harding <[email protected]>
---
RESEND: add wireless mailing list to CC's (requested by Kalle)
drivers/net/wireless/rsi/rsi_91x_hal.c | 13 +++++++------
drivers/net/wireless/rsi/rsi_91x_sdio.c | 9 +++++++--
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
index 1176de646942..839ebdd602df 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -641,7 +641,7 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, u8 *addr, u32 size)
u32 cmd_addr;
u16 cmd_resp, cmd_req;
u8 *str;
- int status;
+ int status, ret;
if (cmd == PING_WRITE) {
cmd_addr = PING_BUFFER_ADDRESS;
@@ -655,12 +655,13 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, u8 *addr, u32 size)
str = "PONG_VALID";
}
- status = hif_ops->load_data_master_write(adapter, cmd_addr, size,
+ ret = hif_ops->load_data_master_write(adapter, cmd_addr, size,
block_size, addr);
- if (status) {
- rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr %0x\n",
- __func__, *addr);
- return status;
+ if (ret) {
+ if (ret != -EINVAL)
+ rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr %0x\n",
+ __func__, *addr);
+ return ret;
}
status = bl_cmd(adapter, cmd_req, cmd_resp, str);
diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index b0cf41195051..b766578b591a 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -20,6 +20,8 @@
#include "rsi_common.h"
#include "rsi_hal.h"
+#define RSI_MAX_BLOCK_SIZE 256
+
/**
* rsi_sdio_set_cmd52_arg() - This function prepares cmd 52 read/write arg.
* @rw: Read/write
@@ -362,7 +364,7 @@ static int rsi_setblocklength(struct rsi_hw *adapter, u32 length)
rsi_dbg(INIT_ZONE, "%s: Setting the block length\n", __func__);
status = sdio_set_block_size(dev->pfunction, length);
- dev->pfunction->max_blksize = 256;
+ dev->pfunction->max_blksize = RSI_MAX_BLOCK_SIZE;
adapter->block_size = dev->pfunction->max_blksize;
rsi_dbg(INFO_ZONE,
@@ -567,9 +569,12 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw *adapter,
{
u32 num_blocks, offset, i;
u16 msb_address, lsb_address;
- u8 temp_buf[block_size];
+ u8 temp_buf[RSI_MAX_BLOCK_SIZE];
int status;
+ if (block_size > RSI_MAX_BLOCK_SIZE)
+ return -EINVAL;
+
num_blocks = instructions_sz / block_size;
msb_address = base_address >> 16;
--
2.7.4
On Sun, Mar 11, 2018 at 09:06:10PM -0500, Larry Finger wrote:
> On 03/11/2018 08:43 PM, Tobin C. Harding wrote:
> >The kernel would like to have all stack VLA usage removed[1]. rsi uses
> >a VLA based on 'blksize'. Elsewhere in the SDIO code maximum block size
> >is defined using a magic number. We can use a pre-processor defined
> >constant and declare the array to maximum size. We add a check before
> >accessing the array in case of programmer error.
> >
> >[1]: https://lkml.org/lkml/2018/3/7/621
> >
> >Signed-off-by: Tobin C. Harding <[email protected]>
> >---
> >
> >RESEND: add wireless mailing list to CC's (requested by Kalle)
> >
> > drivers/net/wireless/rsi/rsi_91x_hal.c | 13 +++++++------
> > drivers/net/wireless/rsi/rsi_91x_sdio.c | 9 +++++++--
> > 2 files changed, 14 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
> >index 1176de646942..839ebdd602df 100644
> >--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
> >+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
> >@@ -641,7 +641,7 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, u8 *addr, u32 size)
> > u32 cmd_addr;
> > u16 cmd_resp, cmd_req;
> > u8 *str;
> >- int status;
> >+ int status, ret;
> > if (cmd == PING_WRITE) {
> > cmd_addr = PING_BUFFER_ADDRESS;
> >@@ -655,12 +655,13 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, u8 *addr, u32 size)
> > str = "PONG_VALID";
> > }
> >- status = hif_ops->load_data_master_write(adapter, cmd_addr, size,
> >+ ret = hif_ops->load_data_master_write(adapter, cmd_addr, size,
> > block_size, addr);
> >- if (status) {
> >- rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr %0x\n",
> >- __func__, *addr);
> >- return status;
> >+ if (ret) {
> >+ if (ret != -EINVAL)
> >+ rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr %0x\n",
> >+ __func__, *addr);
> >+ return ret;
> > }
> > status = bl_cmd(adapter, cmd_req, cmd_resp, str);
> >diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> >index b0cf41195051..b766578b591a 100644
> >--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
> >+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> >@@ -20,6 +20,8 @@
> > #include "rsi_common.h"
> > #include "rsi_hal.h"
> >+#define RSI_MAX_BLOCK_SIZE 256
> >+
> > /**
> > * rsi_sdio_set_cmd52_arg() - This function prepares cmd 52 read/write arg.
> > * @rw: Read/write
> >@@ -362,7 +364,7 @@ static int rsi_setblocklength(struct rsi_hw *adapter, u32 length)
> > rsi_dbg(INIT_ZONE, "%s: Setting the block length\n", __func__);
> > status = sdio_set_block_size(dev->pfunction, length);
> >- dev->pfunction->max_blksize = 256;
> >+ dev->pfunction->max_blksize = RSI_MAX_BLOCK_SIZE;
> > adapter->block_size = dev->pfunction->max_blksize;
> > rsi_dbg(INFO_ZONE,
> >@@ -567,9 +569,12 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw *adapter,
> > {
> > u32 num_blocks, offset, i;
> > u16 msb_address, lsb_address;
> >- u8 temp_buf[block_size];
> >+ u8 temp_buf[RSI_MAX_BLOCK_SIZE];
> > int status;
> >+ if (block_size > RSI_MAX_BLOCK_SIZE)
> >+ return -EINVAL;
> >+
> > num_blocks = instructions_sz / block_size;
> > msb_address = base_address >> 16;
>
> I am not giving this patch a negative review, but my solution to the same
> problem has been to change the on-stack array into a u8 pointer, use
> kmalloc() to assign the space, and then free that space at the end. That way
> large stack allocations are avoided, with a minimum of changes.
Your idea is better Larry, have you got a patch done already or do you
want me to knock one up?
thanks,
Tobin.
tcharding <[email protected]> wrote:
> The kernel would like to have all stack VLA usage removed[1]. rsi uses
> a VLA based on 'blksize'. Elsewhere in the SDIO code maximum block size
> is defined using a magic number. We can use a pre-processor defined
> constant and declare the array to maximum size. We add a check before
> accessing the array in case of programmer error.
>
> [1]: https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Tobin C. Harding <[email protected]>
Tobin, your name in patchwork.kernel.org is just "tcharding" then it should be
"Tobin C. Harding". Patchwork is braindead in a way as it takes the name from
it's database instead of the From header of the patch in question.
I can fix that manually but it would be helpful if you could register to
patchwork and fix your name during registration. You have only one chance to
fix your name (another braindead feature!) so be careful :)
--
https://patchwork.kernel.org/patch/10274983/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
"Tobin C. Harding" <[email protected]> wrote:
> The kernel would like to have all stack VLA usage removed[1]. rsi uses
> a VLA based on 'blksize'. Elsewhere in the SDIO code maximum block size
> is defined using a magic number. We can use a pre-processor defined
> constant and declare the array to maximum size. We add a check before
> accessing the array in case of programmer error.
>
> [1]: https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Tobin C. Harding <[email protected]>
There were conflicts. Can you rebase on top of wireless-drivers-next and
resend, please?
Recorded preimage for 'drivers/net/wireless/rsi/rsi_91x_sdio.c'
error: Failed to merge in the changes.
Applying: rsi: Remove stack VLA usage
Using index info to reconstruct a base tree...
M drivers/net/wireless/rsi/rsi_91x_hal.c
M drivers/net/wireless/rsi/rsi_91x_sdio.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/net/wireless/rsi/rsi_91x_sdio.c
CONFLICT (content): Merge conflict in drivers/net/wireless/rsi/rsi_91x_sdio.c
Auto-merging drivers/net/wireless/rsi/rsi_91x_hal.c
Patch failed at 0001 rsi: Remove stack VLA usage
The copy of the patch that failed is found in: .git/rebase-apply/patch
Patch set to Changes Requested.
--
https://patchwork.kernel.org/patch/10274983/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 03/11/2018 08:43 PM, Tobin C. Harding wrote:
> The kernel would like to have all stack VLA usage removed[1]. rsi uses
> a VLA based on 'blksize'. Elsewhere in the SDIO code maximum block size
> is defined using a magic number. We can use a pre-processor defined
> constant and declare the array to maximum size. We add a check before
> accessing the array in case of programmer error.
>
> [1]: https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Tobin C. Harding <[email protected]>
> ---
>
> RESEND: add wireless mailing list to CC's (requested by Kalle)
>
> drivers/net/wireless/rsi/rsi_91x_hal.c | 13 +++++++------
> drivers/net/wireless/rsi/rsi_91x_sdio.c | 9 +++++++--
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
> index 1176de646942..839ebdd602df 100644
> --- a/drivers/net/wireless/rsi/rsi_91x_hal.c
> +++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
> @@ -641,7 +641,7 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, u8 *addr, u32 size)
> u32 cmd_addr;
> u16 cmd_resp, cmd_req;
> u8 *str;
> - int status;
> + int status, ret;
>
> if (cmd == PING_WRITE) {
> cmd_addr = PING_BUFFER_ADDRESS;
> @@ -655,12 +655,13 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, u8 *addr, u32 size)
> str = "PONG_VALID";
> }
>
> - status = hif_ops->load_data_master_write(adapter, cmd_addr, size,
> + ret = hif_ops->load_data_master_write(adapter, cmd_addr, size,
> block_size, addr);
> - if (status) {
> - rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr %0x\n",
> - __func__, *addr);
> - return status;
> + if (ret) {
> + if (ret != -EINVAL)
> + rsi_dbg(ERR_ZONE, "%s: Unable to write blk at addr %0x\n",
> + __func__, *addr);
> + return ret;
> }
>
> status = bl_cmd(adapter, cmd_req, cmd_resp, str);
> diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> index b0cf41195051..b766578b591a 100644
> --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
> +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> @@ -20,6 +20,8 @@
> #include "rsi_common.h"
> #include "rsi_hal.h"
>
> +#define RSI_MAX_BLOCK_SIZE 256
> +
> /**
> * rsi_sdio_set_cmd52_arg() - This function prepares cmd 52 read/write arg.
> * @rw: Read/write
> @@ -362,7 +364,7 @@ static int rsi_setblocklength(struct rsi_hw *adapter, u32 length)
> rsi_dbg(INIT_ZONE, "%s: Setting the block length\n", __func__);
>
> status = sdio_set_block_size(dev->pfunction, length);
> - dev->pfunction->max_blksize = 256;
> + dev->pfunction->max_blksize = RSI_MAX_BLOCK_SIZE;
> adapter->block_size = dev->pfunction->max_blksize;
>
> rsi_dbg(INFO_ZONE,
> @@ -567,9 +569,12 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw *adapter,
> {
> u32 num_blocks, offset, i;
> u16 msb_address, lsb_address;
> - u8 temp_buf[block_size];
> + u8 temp_buf[RSI_MAX_BLOCK_SIZE];
> int status;
>
> + if (block_size > RSI_MAX_BLOCK_SIZE)
> + return -EINVAL;
> +
> num_blocks = instructions_sz / block_size;
> msb_address = base_address >> 16;
I am not giving this patch a negative review, but my solution to the same
problem has been to change the on-stack array into a u8 pointer, use kmalloc()
to assign the space, and then free that space at the end. That way large stack
allocations are avoided, with a minimum of changes.
Larry
>
>