| Message ID | 20231012-strncpy-drivers-net-hamradio-baycom_epp-c-v1-1-8f4097538ee4@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | hamradio: replace deprecated strncpy with strscpy | expand |
On Thu, Oct 12, 2023 at 09:33:32PM +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. > > We expect both hi.data.modename and hi.data.drivername to be > NUL-terminated but not necessarily NUL-padded which is evident by its > usage with sprintf: > | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s", > | bc->cfg.intclk ? "int" : "ext", > | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps, > | bc->cfg.loopback ? ",loopback" : ""); > > Note that this data is copied out to userspace with: > | if (copy_to_user(data, &hi, sizeof(hi))) > ... however, the data was also copied FROM the user here: > | if (copy_from_user(&hi, data, sizeof(hi))) Thanks Justin, I see that too. Perhaps I am off the mark here, and perhaps it's out of scope for this patch, but I do think it would be nicer if the kernel only sent intended data to user-space, even if any unintended payload came from user-space. > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> ...
On Sun, Oct 15, 2023 at 05:06:19PM +0200, Simon Horman wrote: > On Thu, Oct 12, 2023 at 09:33:32PM +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. > > > > We expect both hi.data.modename and hi.data.drivername to be > > NUL-terminated but not necessarily NUL-padded which is evident by its > > usage with sprintf: > > | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s", > > | bc->cfg.intclk ? "int" : "ext", > > | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps, > > | bc->cfg.loopback ? ",loopback" : ""); > > > > Note that this data is copied out to userspace with: > > | if (copy_to_user(data, &hi, sizeof(hi))) > > ... however, the data was also copied FROM the user here: > > | if (copy_from_user(&hi, data, sizeof(hi))) > > Thanks Justin, > > I see that too. > > Perhaps I am off the mark here, and perhaps it's out of scope for this > patch, but I do think it would be nicer if the kernel only sent > intended data to user-space, even if any unintended payload came > from user-space. > It's kind of normal to pass user space data back to itself. We generally only worry about info leaks. regards, dan carpenter
On October 15, 2023 10:47:53 PM PDT, Dan Carpenter <dan.carpenter@linaro.org> wrote: >On Sun, Oct 15, 2023 at 05:06:19PM +0200, Simon Horman wrote: >> On Thu, Oct 12, 2023 at 09:33:32PM +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. >> > >> > We expect both hi.data.modename and hi.data.drivername to be >> > NUL-terminated but not necessarily NUL-padded which is evident by its >> > usage with sprintf: >> > | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s", >> > | bc->cfg.intclk ? "int" : "ext", >> > | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps, >> > | bc->cfg.loopback ? ",loopback" : ""); >> > >> > Note that this data is copied out to userspace with: >> > | if (copy_to_user(data, &hi, sizeof(hi))) >> > ... however, the data was also copied FROM the user here: >> > | if (copy_from_user(&hi, data, sizeof(hi))) >> >> Thanks Justin, >> >> I see that too. >> >> Perhaps I am off the mark here, and perhaps it's out of scope for this >> patch, but I do think it would be nicer if the kernel only sent >> intended data to user-space, even if any unintended payload came >> from user-space. >> > >It's kind of normal to pass user space data back to itself. We >generally only worry about info leaks. True but since this used to zero the rest of the buffet, let's just keep that behavior and use strscpy_pad(). -Kees
On October 16, 2023 10:01:20 AM PDT, Kees Cook <kees@kernel.org> wrote: > > >On October 15, 2023 10:47:53 PM PDT, Dan Carpenter <dan.carpenter@linaro.org> wrote: >>On Sun, Oct 15, 2023 at 05:06:19PM +0200, Simon Horman wrote: >>> On Thu, Oct 12, 2023 at 09:33:32PM +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. >>> > >>> > We expect both hi.data.modename and hi.data.drivername to be >>> > NUL-terminated but not necessarily NUL-padded which is evident by its >>> > usage with sprintf: >>> > | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s", >>> > | bc->cfg.intclk ? "int" : "ext", >>> > | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps, >>> > | bc->cfg.loopback ? ",loopback" : ""); >>> > >>> > Note that this data is copied out to userspace with: >>> > | if (copy_to_user(data, &hi, sizeof(hi))) >>> > ... however, the data was also copied FROM the user here: >>> > | if (copy_from_user(&hi, data, sizeof(hi))) >>> >>> Thanks Justin, >>> >>> I see that too. >>> >>> Perhaps I am off the mark here, and perhaps it's out of scope for this >>> patch, but I do think it would be nicer if the kernel only sent >>> intended data to user-space, even if any unintended payload came >>> from user-space. >>> >> >>It's kind of normal to pass user space data back to itself. We >>generally only worry about info leaks. > >True but since this used to zero the rest of the buffet, let's just keep that behavior and use strscpy_pad(). I'm calling all byte arrays a "buffet" from now on. ;)
On 17/10/23 04:03, Kees Cook wrote: > > > On October 16, 2023 10:01:20 AM PDT, Kees Cook <kees@kernel.org> wrote: >> >> >> On October 15, 2023 10:47:53 PM PDT, Dan Carpenter <dan.carpenter@linaro.org> wrote: >>> On Sun, Oct 15, 2023 at 05:06:19PM +0200, Simon Horman wrote: >>>> On Thu, Oct 12, 2023 at 09:33:32PM +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. >>>>> >>>>> We expect both hi.data.modename and hi.data.drivername to be >>>>> NUL-terminated but not necessarily NUL-padded which is evident by its >>>>> usage with sprintf: >>>>> | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s", >>>>> | bc->cfg.intclk ? "int" : "ext", >>>>> | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps, >>>>> | bc->cfg.loopback ? ",loopback" : ""); >>>>> >>>>> Note that this data is copied out to userspace with: >>>>> | if (copy_to_user(data, &hi, sizeof(hi))) >>>>> ... however, the data was also copied FROM the user here: >>>>> | if (copy_from_user(&hi, data, sizeof(hi))) >>>> >>>> Thanks Justin, >>>> >>>> I see that too. >>>> >>>> Perhaps I am off the mark here, and perhaps it's out of scope for this >>>> patch, but I do think it would be nicer if the kernel only sent >>>> intended data to user-space, even if any unintended payload came >>>> from user-space. >>>> >>> >>> It's kind of normal to pass user space data back to itself. We >>> generally only worry about info leaks. >> >> True but since this used to zero the rest of the buffet, let's just keep that behavior and use strscpy_pad(). > > I'm calling all byte arrays a "buffet" from now on. ;) > A tasteful change to the sauce I think. ;)
On Mon, Oct 16, 2023 at 3:18 PM Hugh Blemings <hugh@blemings.org> wrote: > > > > On 17/10/23 04:03, Kees Cook wrote: > > > > > > On October 16, 2023 10:01:20 AM PDT, Kees Cook <kees@kernel.org> wrote: > >> > >> > >> On October 15, 2023 10:47:53 PM PDT, Dan Carpenter <dan.carpenter@linaro.org> wrote: > >>> On Sun, Oct 15, 2023 at 05:06:19PM +0200, Simon Horman wrote: > >>>> On Thu, Oct 12, 2023 at 09:33:32PM +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. > >>>>> > >>>>> We expect both hi.data.modename and hi.data.drivername to be > >>>>> NUL-terminated but not necessarily NUL-padded which is evident by its > >>>>> usage with sprintf: > >>>>> | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s", > >>>>> | bc->cfg.intclk ? "int" : "ext", > >>>>> | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps, > >>>>> | bc->cfg.loopback ? ",loopback" : ""); > >>>>> > >>>>> Note that this data is copied out to userspace with: > >>>>> | if (copy_to_user(data, &hi, sizeof(hi))) > >>>>> ... however, the data was also copied FROM the user here: > >>>>> | if (copy_from_user(&hi, data, sizeof(hi))) > >>>> > >>>> Thanks Justin, > >>>> > >>>> I see that too. > >>>> > >>>> Perhaps I am off the mark here, and perhaps it's out of scope for this > >>>> patch, but I do think it would be nicer if the kernel only sent > >>>> intended data to user-space, even if any unintended payload came > >>>> from user-space. > >>>> > >>> > >>> It's kind of normal to pass user space data back to itself. We > >>> generally only worry about info leaks. > >> > >> True but since this used to zero the rest of the buffet, let's just keep that behavior and use strscpy_pad(). > > > > I'm calling all byte arrays a "buffet" from now on. ;) > > > A tasteful change to the sauce I think. ;) Just perfect that this is happening on a **ham**radio driver.
diff --git a/drivers/net/hamradio/baycom_epp.c b/drivers/net/hamradio/baycom_epp.c index 83ff882f5d97..30a0fbb12b9c 100644 --- a/drivers/net/hamradio/baycom_epp.c +++ b/drivers/net/hamradio/baycom_epp.c @@ -1074,7 +1074,7 @@ static int baycom_siocdevprivate(struct net_device *dev, struct ifreq *ifr, return 0; case HDLCDRVCTL_DRIVERNAME: - strncpy(hi.data.drivername, "baycom_epp", sizeof(hi.data.drivername)); + strscpy(hi.data.drivername, "baycom_epp", sizeof(hi.data.drivername)); break; case HDLCDRVCTL_GETMODE: @@ -1091,7 +1091,7 @@ static int baycom_siocdevprivate(struct net_device *dev, struct ifreq *ifr, return baycom_setmode(bc, hi.data.modename); case HDLCDRVCTL_MODELIST: - strncpy(hi.data.modename, "intclk,extclk,intmodem,extmodem,divider=x", + strscpy(hi.data.modename, "intclk,extclk,intmodem,extmodem,divider=x", sizeof(hi.data.modename)); break;
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect both hi.data.modename and hi.data.drivername to be NUL-terminated but not necessarily NUL-padded which is evident by its usage with sprintf: | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s", | bc->cfg.intclk ? "int" : "ext", | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps, | bc->cfg.loopback ? ",loopback" : ""); Note that this data is copied out to userspace with: | if (copy_to_user(data, &hi, sizeof(hi))) ... however, the data was also copied FROM the user here: | if (copy_from_user(&hi, data, sizeof(hi))) Considering the above, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] 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. Also, there are 33 instances of trailing whitespace in this file alone. I've opted to not remove them in this patch. --- drivers/net/hamradio/baycom_epp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231012-strncpy-drivers-net-hamradio-baycom_epp-c-6e11c9483b9f Best regards, -- Justin Stitt <justinstitt@google.com>