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, 1 Aug 2023 16:30:48 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Jessica Clarke <jrtc27@jrtc27.com>
Cc:     Justin Stitt <justinstitt@google.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy
Message-ID: <202308011630.913CE919@keescook>
References: <20230801-arch-riscv-kernel-v1-1-2b3f2dc0bc61@google.com>
 <61DD603A-3D86-462F-B795-7EDE3A59ABBE@jrtc27.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <61DD603A-3D86-462F-B795-7EDE3A59ABBE@jrtc27.com>
Precedence: bulk

On Wed, Aug 02, 2023 at 12:02:11AM +0100, Jessica Clarke wrote:
> On 1 Aug 2023, at 22:14, Justin Stitt <justinstitt@google.com> wrote:
> > 
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> > 
> > A suitable replacement is `strscpy` [2] due to the fact that it
> > guarantees NUL-termination on its destination buffer argument which is
> > _not_ the case for `strncpy`!
> > 
> > The `sv_type` buffer is declared with a size of 16 which is then
> > followed by some `strncpy` calls to populate the buffer with one of:
> > "sv32", "sv57", "sv48", "sv39" or "none". Hard-coding the max length as 5 is
> > error-prone and involves counting the number of characters (and
> > hopefully not forgetting to count the NUL-byte) in the raw string.
> > 
> > Using a pre-determined max length in combination with `strscpy` provides
> > a cleaner, less error-prone as well as a less ambiguous implementation.
> > `strscpy` guarantees that it's destination buffer is NUL-terminated even
> > if it's source argument exceeds the max length as defined by the third
> > argument.
> 
> I would imagine you’d want a BUG_ON() rather than silent truncation if
> that ever happened (well, silent if you ignore it then printing the
> truncated string).
> 
> Though really you just want a static_strcpy that looks at sizeof* for
> source and destination and fails to build if it doesn’t fit; there’s no
> reason this needs to be found at run time.

FWIW, under CONFIG_FORTIFY_SOURCE, strscpy() does try to just fold away
to a static strcpy when sizes are provably safe, etc.

-- 
Kees Cook