`strncpy` is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
`buf` is used in this context as a data buffer with 64 bytes of memory
to be occupied by capi_manufakturer.
We see the caller capi20_get_manufacturer() passes data.manufacturer as
its `buf` argument which is then later passed over to user space. Due to
this, let's keep the NUL-padding that strncpy provided by using
strscpy_pad so as to not leak any stack data.
| cdev->errcode = capi20_get_manufacturer(data.contr, data.manufacturer);
| if (cdev->errcode)
| return -EIO;
|
| if (copy_to_user(argp, data.manufacturer,
| sizeof(data.manufacturer)))
| return -EFAULT;
Perhaps this would also be a good instance to use `strtomem_pad` for but
in my testing the compiler was not able to determine the size of `buf`
-- even with all the hints.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
---
drivers/isdn/capi/kcapi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c
index ae24848af233..136ba9fe55e0 100644
--- a/drivers/isdn/capi/kcapi.c
+++ b/drivers/isdn/capi/kcapi.c
@@ -732,7 +732,7 @@ u16 capi20_get_manufacturer(u32 contr, u8 buf[CAPI_MANUFACTURER_LEN])
u16 ret;
if (contr == 0) {
- strncpy(buf, capi_manufakturer, CAPI_MANUFACTURER_LEN);
+ strscpy_pad(buf, capi_manufakturer, CAPI_MANUFACTURER_LEN);
return CAPI_NOERROR;
}
@@ -740,7 +740,7 @@ u16 capi20_get_manufacturer(u32 contr, u8 buf[CAPI_MANUFACTURER_LEN])
ctr = get_capi_ctr_by_nr(contr);
if (ctr && ctr->state == CAPI_CTR_RUNNING) {
- strncpy(buf, ctr->manu, CAPI_MANUFACTURER_LEN);
+ strscpy_pad(buf, ctr->manu, CAPI_MANUFACTURER_LEN);
ret = CAPI_NOERROR;
} else
ret = CAPI_REGNOTINSTALLED;
---
base-commit: 2cf0f715623872823a72e451243bbf555d10d032
change-id: 20230922-strncpy-drivers-isdn-capi-kcapi-c-516f17f59684
Best regards,
--
Justin Stitt <[email protected]>
On Fri, Sep 22, 2023 at 11:49:14AM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> `buf` is used in this context as a data buffer with 64 bytes of memory
> to be occupied by capi_manufakturer.
>
> We see the caller capi20_get_manufacturer() passes data.manufacturer as
> its `buf` argument which is then later passed over to user space. Due to
> this, let's keep the NUL-padding that strncpy provided by using
> strscpy_pad so as to not leak any stack data.
> | cdev->errcode = capi20_get_manufacturer(data.contr, data.manufacturer);
> | if (cdev->errcode)
> | return -EIO;
> |
> | if (copy_to_user(argp, data.manufacturer,
> | sizeof(data.manufacturer)))
> | return -EFAULT;
Yup, strongly agreed: this needs the padding. I actually wonder if a
follow-up patch might be a good idea here, just for robustness:
capi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
struct capidev *cdev = file->private_data;
- capi_ioctl_struct data;
+ capi_ioctl_struct data = { };
>
> Perhaps this would also be a good instance to use `strtomem_pad` for but
> in my testing the compiler was not able to determine the size of `buf`
> -- even with all the hints.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Fri, 22 Sep 2023 11:49:14 +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> `buf` is used in this context as a data buffer with 64 bytes of memory
> to be occupied by capi_manufakturer.
>
> [...]
Applied to for-next/hardening, thanks!
[1/1] isdn: kcapi: replace deprecated strncpy with strscpy_pad
https://git.kernel.org/kees/c/69cee158c9b0
Take care,
--
Kees Cook