[PATCH] net: stmmac: Avoid VLA usage

[PATCH] net: stmmac: Avoid VLA usage

[Kees Cook]

In the quest to remove all stack VLAs from the kernel[1], this switches
the "status" stack buffer to use the existing small (8) upper bound on
how many queues can be checked for DMA, and adds a sanity-check just to
make sure it doesn't operate under pathological conditions.

[1] http://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b65e2d144698..19bdc23fa314 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2045,7 +2045,11 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 				tx_channel_count : rx_channel_count;
 	u32 chan;
 	bool poll_scheduled = false;
-	int status[channels_to_check];
+	int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
+
+	/* Make sure we never check beyond our status buffer. */
+	if (WARN_ON_ONCE(channels_to_check > ARRAY_SIZE(status)))
+		channels_to_check = ARRAY_SIZE(status);
 
 	/* Each DMA channel can be used for rx and tx simultaneously, yet
 	 * napi_struct is embedded in struct stmmac_rx_queue rather than in a
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH] net: stmmac: Avoid VLA usage

[Jose Abreu]

Hi Kees,

On 01-05-2018 22:01, Kees Cook wrote:
> In the quest to remove all stack VLAs from the kernel[1], this switches
> the "status" stack buffer to use the existing small (8) upper bound on
> how many queues can be checked for DMA, and adds a sanity-check just to
> make sure it doesn't operate under pathological conditions.
>
> [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_CA-2B55aFzCG-2DzNmZwX4A2FQpadafLfEzK6CC-3DqPXydAacU1RqZWA-40mail.gmail.com&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=TBD6a7UY2VbpPmV9LOW_eHAyg8uPq1ZPDhq93VROTVE&s=4fvOST1HhWmZ4lThQe-dHCJYEXNOwey00BCXOWm8tKo&e=
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
>

I rather prefer the variables declaration in reverse-tree order,
but thats just a minor pick.

Reviewed-by: Jose Abreu <joabreu@synopsys.com>

Thanks and Best Regards,
Jose Miguel Abreu

PS: Is VLA warning switch in gcc already active? Because I didn't
see this warning in my builds.

Re: [PATCH] net: stmmac: Avoid VLA usage

[Kees Cook]

On Wed, May 2, 2018 at 1:54 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> Hi Kees,
>
> On 01-05-2018 22:01, Kees Cook wrote:
>> In the quest to remove all stack VLAs from the kernel[1], this switches
>> the "status" stack buffer to use the existing small (8) upper bound on
>> how many queues can be checked for DMA, and adds a sanity-check just to
>> make sure it doesn't operate under pathological conditions.
>>
>> [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_CA-2B55aFzCG-2DzNmZwX4A2FQpadafLfEzK6CC-3DqPXydAacU1RqZWA-40mail.gmail.com&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=TBD6a7UY2VbpPmV9LOW_eHAyg8uPq1ZPDhq93VROTVE&s=4fvOST1HhWmZ4lThQe-dHCJYEXNOwey00BCXOWm8tKo&e=
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>
> I rather prefer the variables declaration in reverse-tree order,
> but thats just a minor pick.

I can explicitly reorder the other variables, if you want?

> Reviewed-by: Jose Abreu <joabreu@synopsys.com>

Thanks!

> PS: Is VLA warning switch in gcc already active? Because I didn't
> see this warning in my builds.

It is not. A bunch of people have been building with KCFLAGS=-Wvla to
find the VLAs and sending patches. Once we get rid of them all, we can
add the flag to the top-level Makefile.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] net: stmmac: Avoid VLA usage

[Jose Abreu]

On 02-05-2018 13:36, Kees Cook wrote:
> On Wed, May 2, 2018 at 1:54 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
>> Hi Kees,
>>
>> On 01-05-2018 22:01, Kees Cook wrote:
>>> In the quest to remove all stack VLAs from the kernel[1], this switches
>>> the "status" stack buffer to use the existing small (8) upper bound on
>>> how many queues can be checked for DMA, and adds a sanity-check just to
>>> make sure it doesn't operate under pathological conditions.
>>>
>>> [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_CA-2B55aFzCG-2DzNmZwX4A2FQpadafLfEzK6CC-3DqPXydAacU1RqZWA-40mail.gmail.com&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=TBD6a7UY2VbpPmV9LOW_eHAyg8uPq1ZPDhq93VROTVE&s=4fvOST1HhWmZ4lThQe-dHCJYEXNOwey00BCXOWm8tKo&e=
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>
>> I rather prefer the variables declaration in reverse-tree order,
>> but thats just a minor pick.
> I can explicitly reorder the other variables, if you want?

No need by me, unless Giuseppe or Alexandre prefer that. Thanks!

Best Regards,
Jose Miguel Abreu

>
>> Reviewed-by: Jose Abreu <joabreu@synopsys.com>
> Thanks!
>
>> PS: Is VLA warning switch in gcc already active? Because I didn't
>> see this warning in my builds.
> It is not. A bunch of people have been building with KCFLAGS=-Wvla to
> find the VLAs and sending patches. Once we get rid of them all, we can
> add the flag to the top-level Makefile.
>
> -Kees
>

Re: [PATCH] net: stmmac: Avoid VLA usage

[Alexandre Torgue]

On 05/02/2018 04:07 PM, Jose Abreu wrote:
> 
> 
> On 02-05-2018 13:36, Kees Cook wrote:
>> On Wed, May 2, 2018 at 1:54 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
>>> Hi Kees,
>>>
>>> On 01-05-2018 22:01, Kees Cook wrote:
>>>> In the quest to remove all stack VLAs from the kernel[1], this switches
>>>> the "status" stack buffer to use the existing small (8) upper bound on
>>>> how many queues can be checked for DMA, and adds a sanity-check just to
>>>> make sure it doesn't operate under pathological conditions.
>>>>
>>>> [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_CA-2B55aFzCG-2DzNmZwX4A2FQpadafLfEzK6CC-3DqPXydAacU1RqZWA-40mail.gmail.com&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=TBD6a7UY2VbpPmV9LOW_eHAyg8uPq1ZPDhq93VROTVE&s=4fvOST1HhWmZ4lThQe-dHCJYEXNOwey00BCXOWm8tKo&e=
>>>>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>
>>> I rather prefer the variables declaration in reverse-tree order,
>>> but thats just a minor pick.
>> I can explicitly reorder the other variables, if you want?
> 
> No need by me, unless Giuseppe or Alexandre prefer that. Thanks!

No need.

> 
> Best Regards,
> Jose Miguel Abreu
> 
>>
>>> Reviewed-by: Jose Abreu <joabreu@synopsys.com>
>> Thanks!
>>
>>> PS: Is VLA warning switch in gcc already active? Because I didn't
>>> see this warning in my builds.
>> It is not. A bunch of people have been building with KCFLAGS=-Wvla to
>> find the VLAs and sending patches. Once we get rid of them all, we can
>> add the flag to the top-level Makefile.
>>
>> -Kees
>>
> 

Re: [PATCH] net: stmmac: Avoid VLA usage

[David Miller]

From: Kees Cook <keescook@chromium.org>
Date: Tue, 1 May 2018 14:01:30 -0700

> In the quest to remove all stack VLAs from the kernel[1], this switches
> the "status" stack buffer to use the existing small (8) upper bound on
> how many queues can be checked for DMA, and adds a sanity-check just to
> make sure it doesn't operate under pathological conditions.
> 
> [1] http://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Applied to net-next, thank you.