Messages in this thread | | | Date | Sat, 23 Sep 2023 20:36:19 -0700 | From | Kees Cook <> | Subject | Re: [PATCH] isdn: kcapi: replace deprecated strncpy with strscpy_pad |
| |
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>
-- Kees Cook
| |