| Message ID | 20240909-strncpy-net-caif-chnl_net-c-v1-1-438eb870c155@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 5905c024a776c7f592581551993817aaf0cd1f36 |
| Headers | show |
| Series | caif: replace deprecated strncpy with strscpy_pad | expand |
On Mon, Sep 09, 2024 at 04:39:28PM -0700, 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. > > Towards the goal of [2], replace strncpy() with an alternative that > guarantees NUL-termination and NUL-padding for the destination buffer. Hi Justin, I am curious to know why the _pad variant was chosen. > 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 [2] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > Cc: Kees Cook <keescook@chromium.org> > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > Note: build-tested only. > --- > net/caif/chnl_net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/caif/chnl_net.c b/net/caif/chnl_net.c > index 47901bd4def1..ff37dceefa26 100644 > --- a/net/caif/chnl_net.c > +++ b/net/caif/chnl_net.c > @@ -347,7 +347,7 @@ static int chnl_net_init(struct net_device *dev) > struct chnl_net *priv; > ASSERT_RTNL(); > priv = netdev_priv(dev); > - strncpy(priv->name, dev->name, sizeof(priv->name)); > + strscpy_pad(priv->name, dev->name); > INIT_LIST_HEAD(&priv->list_field); > return 0; > } > > --- > base-commit: bc83b4d1f08695e85e85d36f7b803da58010161d > change-id: 20240909-strncpy-net-caif-chnl_net-c-a505e955e697 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> > >
Hi, On Tue, Sep 10, 2024 at 2:37 AM Simon Horman <horms@kernel.org> wrote: > > On Mon, Sep 09, 2024 at 04:39:28PM -0700, 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. > > > > Towards the goal of [2], replace strncpy() with an alternative that > > guarantees NUL-termination and NUL-padding for the destination buffer. > > Hi Justin, > > I am curious to know why the _pad variant was chosen. I chose the _pad variant as it matches the behavior of strncpy in this context, ensuring minimal functional change. I think the point you're trying to get at is that the net_device should be zero allocated to begin with -- rendering all thus NUL-padding superfluous. I have some questions out of curiosity: 1) do all control paths leading here zero-allocate the net_device struct? and 2) does it matter that this private data be NUL-padded (I assume not). With all that being said, I'd be happy to send a v2 using the regular strscpy variant if needed. > > > 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 [2] > > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > > Cc: Kees Cook <keescook@chromium.org> > > Cc: linux-hardening@vger.kernel.org > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > Note: build-tested only. > > --- > > net/caif/chnl_net.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/caif/chnl_net.c b/net/caif/chnl_net.c > > index 47901bd4def1..ff37dceefa26 100644 > > --- a/net/caif/chnl_net.c > > +++ b/net/caif/chnl_net.c > > @@ -347,7 +347,7 @@ static int chnl_net_init(struct net_device *dev) > > struct chnl_net *priv; > > ASSERT_RTNL(); > > priv = netdev_priv(dev); > > - strncpy(priv->name, dev->name, sizeof(priv->name)); > > + strscpy_pad(priv->name, dev->name); > > INIT_LIST_HEAD(&priv->list_field); > > return 0; > > } > > > > --- > > base-commit: bc83b4d1f08695e85e85d36f7b803da58010161d > > change-id: 20240909-strncpy-net-caif-chnl_net-c-a505e955e697 > > > > Best regards, > > -- > > Justin Stitt <justinstitt@google.com> > > > > I appreciate the review. Thanks Justin
On Thu, Sep 12, 2024 at 1:43 PM Justin Stitt <justinstitt@google.com> wrote: > > Hi, > > On Tue, Sep 10, 2024 at 2:37 AM Simon Horman <horms@kernel.org> wrote: > > > > On Mon, Sep 09, 2024 at 04:39:28PM -0700, 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. > > > > > > Towards the goal of [2], replace strncpy() with an alternative that > > > guarantees NUL-termination and NUL-padding for the destination buffer. > > > > Hi Justin, > > > > I am curious to know why the _pad variant was chosen. > > I chose the _pad variant as it matches the behavior of strncpy in this > context, ensuring minimal functional change. I think the point you're > trying to get at is that the net_device should be zero allocated to > begin with -- rendering all thus NUL-padding superfluous. I have some > questions out of curiosity: 1) do all control paths leading here > zero-allocate the net_device struct? and 2) does it matter that this > private data be NUL-padded (I assume not). > > With all that being said, I'd be happy to send a v2 using the regular > strscpy variant if needed. I just saw [1] so let's go with that, obviously. > > > > > > 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 [2] > > > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: linux-hardening@vger.kernel.org > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > > --- > > > Note: build-tested only. > > > --- > > > net/caif/chnl_net.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/net/caif/chnl_net.c b/net/caif/chnl_net.c > > > index 47901bd4def1..ff37dceefa26 100644 > > > --- a/net/caif/chnl_net.c > > > +++ b/net/caif/chnl_net.c > > > @@ -347,7 +347,7 @@ static int chnl_net_init(struct net_device *dev) > > > struct chnl_net *priv; > > > ASSERT_RTNL(); > > > priv = netdev_priv(dev); > > > - strncpy(priv->name, dev->name, sizeof(priv->name)); > > > + strscpy_pad(priv->name, dev->name); > > > INIT_LIST_HEAD(&priv->list_field); > > > return 0; > > > } > > > > > > --- > > > base-commit: bc83b4d1f08695e85e85d36f7b803da58010161d > > > change-id: 20240909-strncpy-net-caif-chnl_net-c-a505e955e697 > > > > > > Best regards, > > > -- > > > Justin Stitt <justinstitt@google.com> > > > > > > > > I appreciate the review. > > Thanks > Justin [1]: https://lore.kernel.org/all/20240911015228.1555779-1-kuba@kernel.org/ Thanks Justin
On Thu, Sep 12, 2024 at 01:47:22PM -0700, Justin Stitt wrote: > On Thu, Sep 12, 2024 at 1:43 PM Justin Stitt <justinstitt@google.com> wrote: > > > > Hi, > > > > On Tue, Sep 10, 2024 at 2:37 AM Simon Horman <horms@kernel.org> wrote: > > > > > > On Mon, Sep 09, 2024 at 04:39:28PM -0700, 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. > > > > > > > > Towards the goal of [2], replace strncpy() with an alternative that > > > > guarantees NUL-termination and NUL-padding for the destination buffer. > > > > > > Hi Justin, > > > > > > I am curious to know why the _pad variant was chosen. > > > > I chose the _pad variant as it matches the behavior of strncpy in this > > context, ensuring minimal functional change. I think the point you're > > trying to get at is that the net_device should be zero allocated to > > begin with -- rendering all thus NUL-padding superfluous. I have some > > questions out of curiosity: 1) do all control paths leading here > > zero-allocate the net_device struct? and 2) does it matter that this > > private data be NUL-padded (I assume not). > > > > With all that being said, I'd be happy to send a v2 using the regular > > strscpy variant if needed. > > I just saw [1] so let's go with that, obviously. Hi Justin, Yes, right, let's go with that. But as I asked some questions, and you provided your own, let me see if I can respond appropriately as although the answers are specific to this patch the questions seem more generally applicable. 1) It seems to me that the priv data is allocated by alloc_netdev_mqs() which makes the allocation using kvzalloc(). So I believe the answer is that the allocation of name is zeroed. My analysis is based on ops->priv_size being passed to rtnl_create_link() by rtnl_create_link(). And ops being registered using rtnl_link_register() by chnl_init_module(). Of course, I could be missing something here. 2) Regarding a requirement to NUL-pad. FWIIW, this is what I had in mind when I asked my question. But perhaps given 1) it is moot. In any event we now know, thanks to Jakub's investigation [1], that the answer must be no. For the trivial reason that name isn't used.
diff --git a/net/caif/chnl_net.c b/net/caif/chnl_net.c index 47901bd4def1..ff37dceefa26 100644 --- a/net/caif/chnl_net.c +++ b/net/caif/chnl_net.c @@ -347,7 +347,7 @@ static int chnl_net_init(struct net_device *dev) struct chnl_net *priv; ASSERT_RTNL(); priv = netdev_priv(dev); - strncpy(priv->name, dev->name, sizeof(priv->name)); + strscpy_pad(priv->name, dev->name); INIT_LIST_HEAD(&priv->list_field); return 0; }
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. Towards the goal of [2], replace strncpy() with an alternative that guarantees NUL-termination and NUL-padding for the destination buffer. 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 [2] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Cc: Kees Cook <keescook@chromium.org> Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Note: build-tested only. --- net/caif/chnl_net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: bc83b4d1f08695e85e85d36f7b803da58010161d change-id: 20240909-strncpy-net-caif-chnl_net-c-a505e955e697 Best regards, -- Justin Stitt <justinstitt@google.com>