Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Wed, 18 Oct 2023 16:35:07 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Jeff Johnson <quic_jjohnson@quicinc.com>
Cc:     Justin Stitt <justinstitt@google.com>,
        Kalle Valo <kvalo@kernel.org>, ath10k@lists.infradead.org,
        linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad
Message-ID: <202310181626.C5BE0C21F@keescook>
References: <20231013-strncpy-drivers-net-wireless-ath-ath10k-mac-c-v1-1-24e40201afa3@google.com>
 <1cfc7c64-439c-437e-af82-7fce1202242d@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1cfc7c64-439c-437e-af82-7fce1202242d@quicinc.com>
Precedence: bulk

On Fri, Oct 13, 2023 at 05:58:03PM -0700, Jeff Johnson wrote:
> On 10/13/2023 1:33 PM, Justin Stitt wrote:
> > strncpy() is deprecated [1] and we should prefer less ambiguous
> > interfaces.
> > 
> > In this case, arvif->u.ap.ssid has its length maintained by
> > arvif->u.ap.ssid_len which indicates it may not need to be
> > NUL-terminated, although by virtue of using strtomem_pad (with NUL-byte
> > pad character) and having a destination size larger than the source,
> > ssid will, incidentally, be NUL-terminated here.
> > 
> > As strtomem_pad() docs say:
> >   * @dest: Pointer of destination character array (marked as __nonstring)
> >   * @src: Pointer to NUL-terminated string
> >   * @pad: Padding character to fill any remaining bytes of @dest after copy
> >   *
> >   * This is a replacement for strncpy() uses where the destination is not
> >   * a NUL-terminated string, but with bounds checking on the source size, and
> >   * an explicit padding character. If padding is not required, use strtomem().
> > 
> > Let's also mark ath10k_vif.u.ap.ssid as __nonstring.
> 
> what criteria is used to determine whether or not to use __nonstring?
> doesn't the use of u8 vs char already communicate that distinction?
> just want to know what other u8 arrays might require this.
> FWIW the documentation referenced by the __nonstring macro explicitly refers
> to "type array of char, signed char, or unsigned char"

The use of __nonstring is for byte arrays that are _not_ expected to be
%NUL terminated. Unfortunately "char" vs "u8" isn't distinguished by the
compiler. All byte arrays are treated as C strings unless __nonstring is
used.

> > It is unclear to me whether padding is strictly necessary. Perhaps we
> > should opt for just strtomem() -- padding certainly doesn't hurt,
> > though.
> 
> concur that padding probably isn't necessary but doesn't hurt, and will
> prevent confusion if looking at this member in a crashdump
> 
> > 
> > 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
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> 
> Either with or without the __nonstring...
> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Yup, it looks like the ssid member is passed around with memcpy()
everywhere else.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook