diff mbox series

[1/8] of: Fix modalias string generation

Message ID 20230301152239.531194-2-miquel.raynal@bootlin.com
State New
Headers
Series nvmem: Let layout drivers be modules |

Commit Message

Miquel Raynal March 1, 2023, 3:22 p.m. UTC 
  The helper generating an OF based modalias (of_device_get_modalias())
works fine, but due to the use of snprintf() internally it needs a
buffer one byte longer than what should be needed just for the entire
string (excluding the '\0'). Most users of this helper are sysfs hooks
providing the modalias string to users. They all provide a PAGE_SIZE
buffer which is way above the number of bytes required to fit the
modalias string and hence do not suffer from this issue.

There is another user though, of_device_request_module(), which is only
called by drivers/usb/common/ulpi.c. This request module function is
faulty, but maybe because in most cases there is an alternative, ULPI
driver users have not noticed it.

In this function, of_device_get_modalias() is called twice. The first
time without buffer just to get the number of bytes required by the
modalias string (excluding the null byte), and a second time, after
buffer allocation, to fill the buffer. The allocation asks for an
additional byte, in order to store the trailing '\0'. However, the
buffer *length* provided to of_device_get_modalias() excludes this extra
byte. The internal use of snprintf() with a length that is exactly the
number of bytes to be written has the effect of using the last available
byte to store a '\0', which then smashes the last character of the
modalias string.

Provide the actual size of the buffer to of_device_get_modalias() to fix
this issue.

Note: the "str[size - 1] = '\0';" line is not really needed as snprintf
will anyway end the string with a null byte, but there is a possibility
that this function might be called on a struct device_node without
compatible, in this case snprintf() would not be executed. So we keep it
just to avoid possible unbounded strings.

Cc: Stephen Boyd <stephen.boyd@linaro.org>
Cc: Peter Chen <peter.chen@nxp.com>
Fixes: 9c829c097f2f ("of: device: Support loading a module with OF based modalias")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/of/device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Rob Herring March 2, 2023, 7:21 p.m. UTC | #1 
On Wed, Mar 1, 2023 at 9:22 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> The helper generating an OF based modalias (of_device_get_modalias())
> works fine, but due to the use of snprintf() internally it needs a
> buffer one byte longer than what should be needed just for the entire
> string (excluding the '\0'). Most users of this helper are sysfs hooks
> providing the modalias string to users. They all provide a PAGE_SIZE
> buffer which is way above the number of bytes required to fit the
> modalias string and hence do not suffer from this issue.
>
> There is another user though, of_device_request_module(), which is only
> called by drivers/usb/common/ulpi.c. This request module function is
> faulty, but maybe because in most cases there is an alternative, ULPI
> driver users have not noticed it.
>
> In this function, of_device_get_modalias() is called twice. The first
> time without buffer just to get the number of bytes required by the
> modalias string (excluding the null byte), and a second time, after
> buffer allocation, to fill the buffer. The allocation asks for an
> additional byte, in order to store the trailing '\0'. However, the
> buffer *length* provided to of_device_get_modalias() excludes this extra
> byte. The internal use of snprintf() with a length that is exactly the
> number of bytes to be written has the effect of using the last available
> byte to store a '\0', which then smashes the last character of the
> modalias string.
>
> Provide the actual size of the buffer to of_device_get_modalias() to fix
> this issue.
>
> Note: the "str[size - 1] = '\0';" line is not really needed as snprintf
> will anyway end the string with a null byte, but there is a possibility
> that this function might be called on a struct device_node without
> compatible, in this case snprintf() would not be executed. So we keep it
> just to avoid possible unbounded strings.
>
> Cc: Stephen Boyd <stephen.boyd@linaro.org>
> Cc: Peter Chen <peter.chen@nxp.com>
> Fixes: 9c829c097f2f ("of: device: Support loading a module with OF based modalias")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/of/device.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Ahh, an off by 2 error. ;)

Reviewed-by: Rob Herring <robh@kernel.org>
diff mbox series

Patch

diff --git a/drivers/of/device.c b/drivers/of/device.c
index c674a13c3055..877f50379fab 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -297,12 +297,15 @@  int of_device_request_module(struct device *dev)
 	if (size < 0)
 		return size;
 
-	str = kmalloc(size + 1, GFP_KERNEL);
+	/* Reserve an additional byte for the trailing '\0' */
+	size++;
+
+	str = kmalloc(size, GFP_KERNEL);
 	if (!str)
 		return -ENOMEM;
 
 	of_device_get_modalias(dev, str, size);
-	str[size] = '\0';
+	str[size - 1] = '\0';
 	ret = request_module(str);
 	kfree(str);