On Thu, Apr 4, 2024 at 4:39 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Thu, Apr 04, 2024 at 03:47:05PM -0700, Justin Stitt wrote:
> > Cc'ing Kees.
> >
> > On Thu, Apr 4, 2024 at 3:33 PM James Bottomley
> > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > But additionally this is a common pattern in SCSI: using strncpy to
> > > zero terminate fields that may be unterminated in the exchange protocol
> > > so we can send them to sysfs or otherwise treat them as strings. That
> > > means we might have this problem in other drivers you've converted ...
>
> That's why we've been doing these one at a time and getting maintainers
> to review them. :) Justin (and the reviewers) have been trying to be
> careful with these, and documenting the rationales, etc, but this is
> kind of why we're doing the conversion: using strncpy is really
> ambiguous as far as showing what an author intended to be happening.
>
> > Correct. Although certain conditions must be met:
> >
> > 1) length argument is larger than source but less than or equal to destination
> > 2) source is not NUL-terminated
> > 3) sizes known at compile-time
> >
> > I think fortified strscpy needs to be a bit more lenient towards
> > source buffer overreads when we know strscpy should just truncate and
> > NUL-terminate.
>
> Okay, so the problem here is that the _source_ fields aren't NUL
> terminated?
>
> struct sas_expander_device {
> ...
> #define SAS_EXPANDER_VENDOR_ID_LEN 8
> char vendor_id[SAS_EXPANDER_VENDOR_ID_LEN+1];
> ...
> };
>
> struct rep_manu_reply {
> ...
> u8 vendor_id[SAS_EXPANDER_VENDOR_ID_LEN];
> ...
> };
>
> Okay, so struct rep_manu_reply needs __nonstring markings, and the
> strscpy()s need to be memcpy()s.
>
rep_manu_reply->xyz is the source, sure we can mark those as
__nonstring but don't we want the source to be potentially
NUL-terminated (says James). Are you suggesting a memcpy() followed by
a manual NUL-byte assignment?
> We've done those kinds of conversions in the past; it just looks like we
> made an analysis mistake here. Sorry about that!
>
> -Kees
>
> --
> Kees Cook
