Agh, typo.
On Thu, Apr 4, 2024 at 5:10 PM Justin Stitt <justinstitt@xxxxxxxxxx> wrote:
>
> 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?
... don't we want the DESTINATION to be ...
>
> > 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
