Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Jul 2023 11:05:23 -0700
From:   Kees Cook <kees@kernel.org>
To:     justinstitt@google.com, Justin Stitt <justinstitt@google.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
CC:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Kees Cook <keescook@chromium.org>,
        Nick Desaulniers <ndesaulniers@google.com>
Subject: Re: [PATCH] net: dsa: remove deprecated strncpy
User-Agent: K-9 Mail for Android
In-Reply-To: <20230718-net-dsa-strncpy-v1-1-e84664747713@google.com>
References: <20230718-net-dsa-strncpy-v1-1-e84664747713@google.com>
Message-ID: <316E4325-6845-4EFC-AAF8-160622C42144@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On July 17, 2023 5:04:19 PM PDT, justinstitt@google=2Ecom wrote:
>`strncpy` is deprecated for use on NUL-terminated destination strings [1]=
=2E
>
>Even call sites utilizing length-bounded destination buffers should
>switch over to using `strtomem` or `strtomem_pad`=2E In this case,
>however, the compiler is unable to determine the size of the `data`
>buffer which renders `strtomem` unusable=2E Due to this, `strscpy`
>should be used=2E
>
>It should be noted that most call sites already zero-initialize the
>destination buffer=2E However, I've opted to use `strscpy_pad` to maintai=
n
>the same exact behavior that `strncpy` produced (zero-padded tail up to
>`len`)=2E
>
>Also see [3]=2E
>
>[1]: www=2Ekernel=2Eorg/doc/html/latest/process/deprecated=2Ehtml#strncpy=
-on-nul-terminated-strings
>[2]: elixir=2Ebootlin=2Ecom/linux/v6=2E3/source/net/ethtool/ioctl=2Ec#L19=
44
>[3]: manpages=2Edebian=2Eorg/testing/linux-manual-4=2E8/strscpy=2E9=2Een=
=2Ehtml
>
>Link: https://github=2Ecom/KSPP/linux/issues/90
>Signed-off-by: Justin Stitt <justinstitt@google=2Ecom>

This looks fine to me=2E I think the _pad variant is overkill (this region=
 is already zero-initialized[1]), but it's a reasonable precaution for robu=
stness=2E

Honestly I find the entire get_strings API to be very fragile given the la=
ck of passing the length of the buffer, instead depending on the string set=
 length lookups in each callback, but refactoring that looks like a ton of =
work for an uncertain benefit=2E

Reviewed-by: Kees Cook <keescook@chromium=2Eorg>

-Kees

[1] https://elixir=2Ebootlin=2Ecom/linux/v6=2E3/source/net/ethtool/ioctl=
=2Ec#L1944


--=20
Kees Cook