caif: replace deprecated strncpy with strscpy_pad

[PATCH] caif: replace deprecated strncpy with strscpy_pad

Posted by Justin Stitt 2 months, 3 weeks ago

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(-)

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>

Re: [PATCH] caif: replace deprecated strncpy with strscpy_pad

Posted by Simon Horman 2 months, 2 weeks ago

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>
> 
>

Re: [PATCH] caif: replace deprecated strncpy with strscpy_pad

Posted by Justin Stitt 2 months, 2 weeks ago

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

Re: [PATCH] caif: replace deprecated strncpy with strscpy_pad

Posted by Justin Stitt 2 months, 2 weeks ago

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

Re: [PATCH] caif: replace deprecated strncpy with strscpy_pad

Posted by Simon Horman 2 months, 2 weeks ago

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.