linux-kernel - Re: [PATCH] caif: replace deprecated strncpy with strscpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240913074324.GA1132019@kernel.org>
Date: Fri, 13 Sep 2024 08:43:24 +0100
From: Simon Horman <horms@...nel.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org, Kees Cook <kees@...nel.org>
Subject: Re: [PATCH] caif: replace deprecated strncpy with strscpy_pad

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@...gle.com> wrote:
> >
> > Hi,
> >
> > On Tue, Sep 10, 2024 at 2:37 AM Simon Horman <horms@...nel.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.