[PATCH] firmware: raspberrypi: Remove VLA usage

[PATCH] firmware: raspberrypi: Remove VLA usage

[Kees Cook]

In the quest to remove all stack VLA usage from the kernel[1], this
removes the VLA in favor of a maximum size and adds a sanity check.
Existing callers of the firmware interface never need more than 24
bytes (struct gpio_set_config). This chooses 32 just to stay ahead
of future growth.

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

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/firmware/raspberrypi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index 0602626bf72d..b80f15214b73 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -21,6 +21,8 @@
 #define MBOX_DATA28(msg)		((msg) & ~0xf)
 #define MBOX_CHAN_PROPERTY		8
 
+#define MAX_RPI_FW_PROP_BUF_SIZE	32
+
 static struct platform_device *rpi_hwmon;
 
 struct rpi_firmware {
@@ -145,11 +147,15 @@ int rpi_firmware_property(struct rpi_firmware *fw,
 	/* Single tags are very small (generally 8 bytes), so the
 	 * stack should be safe.
 	 */
-	u8 data[buf_size + sizeof(struct rpi_firmware_property_tag_header)];
+	u8 data[sizeof(struct rpi_firmware_property_tag_header) +
+		MAX_RPI_FW_PROP_BUF_SIZE];
 	struct rpi_firmware_property_tag_header *header =
 		(struct rpi_firmware_property_tag_header *)data;
 	int ret;
 
+	if (WARN_ON(buf_size > sizeof(data) - sizeof(*header)))
+		return -EINVAL;
+
 	header->tag = tag;
 	header->buf_size = buf_size;
 	header->req_resp_size = 0;
-- 
2.17.1


-- 
Kees Cook
Pixel Security

[PATCH] fixup! firmware: raspberrypi: Remove VLA usage

[Eric Anholt]

Kees - with this fix to your patch, the kernel boots again (otherwise,
the FW would try to parse the uninitialized bits of stack and throw
errors).  If you're good with me squashing this in, I'll do so and
send it to -next.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/firmware/raspberrypi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index b80f15214b73..a200a2174611 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -162,7 +162,7 @@ int rpi_firmware_property(struct rpi_firmware *fw,
 	memcpy(data + sizeof(struct rpi_firmware_property_tag_header),
 	       tag_data, buf_size);
 
-	ret = rpi_firmware_property_list(fw, &data, sizeof(data));
+	ret = rpi_firmware_property_list(fw, &data, buf_size + sizeof(*header));
 	memcpy(tag_data,
 	       data + sizeof(struct rpi_firmware_property_tag_header),
 	       buf_size);
-- 
2.18.0

Re: [PATCH] fixup! firmware: raspberrypi: Remove VLA usage

[Kees Cook]

On Mon, Jul 2, 2018 at 12:45 PM, Eric Anholt <eric@anholt.net> wrote:
> Kees - with this fix to your patch, the kernel boots again (otherwise,
> the FW would try to parse the uninitialized bits of stack and throw
> errors).  If you're good with me squashing this in, I'll do so and
> send it to -next.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/firmware/raspberrypi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index b80f15214b73..a200a2174611 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -162,7 +162,7 @@ int rpi_firmware_property(struct rpi_firmware *fw,
>         memcpy(data + sizeof(struct rpi_firmware_property_tag_header),
>                tag_data, buf_size);
>
> -       ret = rpi_firmware_property_list(fw, &data, sizeof(data));
> +       ret = rpi_firmware_property_list(fw, &data, buf_size + sizeof(*header));
>         memcpy(tag_data,
>                data + sizeof(struct rpi_firmware_property_tag_header),
>                buf_size);

Eek! Yes, thank you. Yeah, please feel free to squash. Thanks for testing!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] fixup! firmware: raspberrypi: Remove VLA usage

[Eric Anholt]

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

Kees Cook <keescook@chromium.org> writes:

> On Mon, Jul 2, 2018 at 12:45 PM, Eric Anholt <eric@anholt.net> wrote:
>> Kees - with this fix to your patch, the kernel boots again (otherwise,
>> the FW would try to parse the uninitialized bits of stack and throw
>> errors).  If you're good with me squashing this in, I'll do so and
>> send it to -next.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>  drivers/firmware/raspberrypi.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
>> index b80f15214b73..a200a2174611 100644
>> --- a/drivers/firmware/raspberrypi.c
>> +++ b/drivers/firmware/raspberrypi.c
>> @@ -162,7 +162,7 @@ int rpi_firmware_property(struct rpi_firmware *fw,
>>         memcpy(data + sizeof(struct rpi_firmware_property_tag_header),
>>                tag_data, buf_size);
>>
>> -       ret = rpi_firmware_property_list(fw, &data, sizeof(data));
>> +       ret = rpi_firmware_property_list(fw, &data, buf_size + sizeof(*header));
>>         memcpy(tag_data,
>>                data + sizeof(struct rpi_firmware_property_tag_header),
>>                buf_size);
>
> Eek! Yes, thank you. Yeah, please feel free to squash. Thanks for testing!

Squashed and sent the PR.  Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]