| Message ID | 20230922-strncpy-drivers-isdn-capi-kcapi-c-v1-1-55fcf8b075fb@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | cba58fcbc4ab75d8814ec43db32d4830670526f8 |
| Headers | show |
| Series | isdn: kcapi: replace deprecated strncpy with strscpy_pad | expand |
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: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org>
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,
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;
`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: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Note: build-tested only. --- drivers/isdn/capi/kcapi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: 2cf0f715623872823a72e451243bbf555d10d032 change-id: 20230922-strncpy-drivers-isdn-capi-kcapi-c-516f17f59684 Best regards, -- Justin Stitt <justinstitt@google.com>