Re: [PATCH] firmware: raspberrypi: Fix firmware calls with large buffers

Re: [PATCH] firmware: raspberrypi: Fix firmware calls with large buffers

[Kees Cook]

On Thu, Nov 15, 2018 at 10:05 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> Hi James,
>
> please look at
> https://www.kernel.org/doc/html/v4.19/process/submitting-patches.html,
> because there are several issues with this patch. Most critical one is
> that i received it not as plain text. Please make sure that send your
> patch with git send-email.

The irony is I had to fight Gmail over your multipart HTML+plain
email. Yay email clients! ;)

> Am 15.11.18 um 14:18 schrieb James Hughes:
> > A previous change (5bfdc1097654) moved away from VLA's
>
> Please use the commit format mentioned in the link above.

And actually, this SHA isn't the upstream SHA. This should be:

a1547e0bca51 ("firmware: raspberrypi: Remove VLA usage")

> > to a fixed maximum size for mailbox data.
> > However, some mailbox calls use larger data buffers
> > than the maximum allowed in that change. This fix therefor

Which ones did this? In the initial change I couldn't find anything
that exceeded 32 bytes. (I'm just curious if I missed something or if
something new appeared.)

> > [...]
> > + /* Some mailboxes can use over 1k bytes. Rather than checking

Up to Eric, but I think the preferred comment style is:

/*
 * lines go here
 */

Otherwise, this seems fine to me. Thanks for getting it fixed!

-- 
Kees Cook

Re: [PATCH] firmware: raspberrypi: Fix firmware calls with large buffers

[Eric Anholt]

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

Kees Cook <keescook@chromium.org> writes:

> On Thu, Nov 15, 2018 at 10:05 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> Hi James,
>>
>> please look at
>> https://www.kernel.org/doc/html/v4.19/process/submitting-patches.html,
>> because there are several issues with this patch. Most critical one is
>> that i received it not as plain text. Please make sure that send your
>> patch with git send-email.
>
> The irony is I had to fight Gmail over your multipart HTML+plain
> email. Yay email clients! ;)
>
>> Am 15.11.18 um 14:18 schrieb James Hughes:
>> > A previous change (5bfdc1097654) moved away from VLA's
>>
>> Please use the commit format mentioned in the link above.
>
> And actually, this SHA isn't the upstream SHA. This should be:
>
> a1547e0bca51 ("firmware: raspberrypi: Remove VLA usage")
>
>> > to a fixed maximum size for mailbox data.
>> > However, some mailbox calls use larger data buffers
>> > than the maximum allowed in that change. This fix therefor
>
> Which ones did this? In the initial change I couldn't find anything
> that exceeded 32 bytes. (I'm just curious if I missed something or if
> something new appeared.)

Nothing in tree used larger, it's that there are firmware transactions
that are bigger.

>> > [...]
>> > + /* Some mailboxes can use over 1k bytes. Rather than checking
>
> Up to Eric, but I think the preferred comment style is:
>
> /*
>  * lines go here
>  */
>
> Otherwise, this seems fine to me. Thanks for getting it fixed!

I have no preference about comment style, I just want fixes to land. :)

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

[PATCH] firmware: raspberrypi: Fix firmware calls with large buffers

[James Hughes]

Commit a1547e0bca51 ("firmware: raspberrypi: Remove VLA usage")
moved away from VLA's to a fixed maximum size for mailbox data.
However, some mailbox calls use larger data buffers
than the maximum allowed in that change. This fix therefor
moves from using fixed buffers to kmalloc to ensure all sizes
are catered for.

There is some documentation, which is somewhat out of date,
on the mailbox calls here :
https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface

v2: Changes to commit message and format only. No code change.

Fixes: a1547e0bca51 ("firmware: raspberrypi: Remove VLA usage")

Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
 drivers/firmware/raspberrypi.c | 35 +++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index a200a2174611..9257bee7860f 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <soc/bcm2835/raspberrypi-firmware.h>
 
 #define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
@@ -21,8 +22,6 @@
 #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 {
@@ -144,28 +143,30 @@ EXPORT_SYMBOL_GPL(rpi_firmware_property_list);
 int rpi_firmware_property(struct rpi_firmware *fw,
 			  u32 tag, void *tag_data, size_t buf_size)
 {
-	/* Single tags are very small (generally 8 bytes), so the
-	 * stack should be safe.
-	 */
-	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;
+	struct rpi_firmware_property_tag_header *header;
 
-	if (WARN_ON(buf_size > sizeof(data) - sizeof(*header)))
-		return -EINVAL;
+	/* Some mailboxes can use over 1k bytes. Rather than checking
+	 * size and using stack or kmalloc depending on requirements,
+	 * just use kmalloc. Mailboxes don't get called enough to worry
+	 * too much about the time taken in the allocation.
+	 */
+	void *data = kmalloc(sizeof(*header) + buf_size, GFP_KERNEL);
 
+	if (!data)
+		return -ENOMEM;
+
+	header = data;
 	header->tag = tag;
 	header->buf_size = buf_size;
 	header->req_resp_size = 0;
-	memcpy(data + sizeof(struct rpi_firmware_property_tag_header),
-	       tag_data, buf_size);
+	memcpy(data + sizeof(*header), tag_data, buf_size);
+
+	ret = rpi_firmware_property_list(fw, data, buf_size + sizeof(*header));
+
+	memcpy(tag_data, data + sizeof(*header), buf_size);
 
-	ret = rpi_firmware_property_list(fw, &data, buf_size + sizeof(*header));
-	memcpy(tag_data,
-	       data + sizeof(struct rpi_firmware_property_tag_header),
-	       buf_size);
+	kfree(data);
 
 	return ret;
 }
-- 
2.17.1

[PATCH] firmware: raspberrypi: Fix firmware calls with large buffers

[Stefan Wahren]

Hi James,

Am 16.11.18 um 15:07 schrieb James Hughes:
> Commit a1547e0bca51 ("firmware: raspberrypi: Remove VLA usage")
> moved away from VLA's to a fixed maximum size for mailbox data.
> However, some mailbox calls use larger data buffers
> than the maximum allowed in that change. This fix therefor
> moves from using fixed buffers to kmalloc to ensure all sizes
> are catered for.
>
> There is some documentation, which is somewhat out of date,
> on the mailbox calls here :
> https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
>
> v2: Changes to commit message and format only. No code change.
>
> Fixes: a1547e0bca51 ("firmware: raspberrypi: Remove VLA usage")
>
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> ---
>  drivers/firmware/raspberrypi.c | 35 +++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)

just some nits. Please place your change log here, not in the commit log.

You also missed the version number in the subject.

>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index a200a2174611..9257bee7860f 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  #include <soc/bcm2835/raspberrypi-firmware.h>
>  
>  #define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
> @@ -21,8 +22,6 @@
>  #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 {
> @@ -144,28 +143,30 @@ EXPORT_SYMBOL_GPL(rpi_firmware_property_list);
>  int rpi_firmware_property(struct rpi_firmware *fw,
>  			  u32 tag, void *tag_data, size_t buf_size)
>  {
> -	/* Single tags are very small (generally 8 bytes), so the
> -	 * stack should be safe.
> -	 */
> -	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;
> +	struct rpi_firmware_property_tag_header *header;

please don't move the declaration of rpi_firmware_property_tag_header
and only remove the assignment.

Thanks
Stefan