strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89
Signed-off-by: Azeem Shaikh <[email protected]>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index db5c9343a3d2..0918d80672bb 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1951,7 +1951,7 @@ static int tda998x_create(struct device *dev)
* offset.
*/
memset(&cec_info, 0, sizeof(cec_info));
- strlcpy(cec_info.type, "tda9950", sizeof(cec_info.type));
+ strscpy(cec_info.type, "tda9950", sizeof(cec_info.type));
cec_info.addr = priv->cec_addr;
cec_info.platform_data = &priv->cec_glue;
cec_info.irq = client->irq;
On Mon, May 22, 2023 at 03:53:50PM +0000, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
> No return values were used, so direct replacement is safe.
...
> memset(&cec_info, 0, sizeof(cec_info));
> - strlcpy(cec_info.type, "tda9950", sizeof(cec_info.type));
> + strscpy(cec_info.type, "tda9950", sizeof(cec_info.type));
Please explain how:
1) a C string can not be NUL terminated.
2) this source string could be longer than I2C_NAME_SIZE (20 bytes)
which is unlikely to ever shrink.
I'm not saying I disagree with the patch, but the boilerplate commit
message isn't correct for this change, and is actually misleading
for what the patch actually is.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, May 22, 2023 at 05:04:09PM +0100, Russell King (Oracle) wrote:
> On Mon, May 22, 2023 at 03:53:50PM +0000, Azeem Shaikh wrote:
> > strlcpy() reads the entire source buffer first.
> > This read may exceed the destination size limit.
> > This is both inefficient and can lead to linear read
> > overflows if a source string is not NUL-terminated [1].
> > In an effort to remove strlcpy() completely [2], replace
> > strlcpy() here with strscpy().
> > No return values were used, so direct replacement is safe.
> ...
> > memset(&cec_info, 0, sizeof(cec_info));
> > - strlcpy(cec_info.type, "tda9950", sizeof(cec_info.type));
> > + strscpy(cec_info.type, "tda9950", sizeof(cec_info.type));
>
> Please explain how:
>
> 1) a C string can not be NUL terminated.
> 2) this source string could be longer than I2C_NAME_SIZE (20 bytes)
> which is unlikely to ever shrink.
Yes, in this case, obviously none of those can happen.
> I'm not saying I disagree with the patch, but the boilerplate commit
> message isn't correct for this change, and is actually misleading
> for what the patch actually is.
One of the common code patterns in the kernel is copying fixed sized
strings (like here), but Linus refused (probably correctly) to allow an
API for that, since we already had "too many" string functions.
The long-term goal here is to replace all use of strlcpy(),
strncpy(), and strcpy() and replace them with strscpy(). The strscpy()
wrapper is already optimized to short-cut for fixed-size dest/src:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/fortify-string.h?h=v6.3#n337
Perhaps this goal needs to be stated in the commit log to be more clear
about cases like this?
-Kees
--
Kees Cook
On Mon, May 22, 2023 at 03:53:50PM +0000, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
> No return values were used, so direct replacement is safe.
>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
>
> Signed-off-by: Azeem Shaikh <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Mon, 22 May 2023 15:53:50 +0000, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
> No return values were used, so direct replacement is safe.
>
> [...]
Applied to for-next/hardening, thanks!
[1/1] drm/i2c: tda998x: Replace all non-returning strlcpy with strscpy
https://git.kernel.org/kees/c/a7aba6fa2750
--
Kees Cook