Re: startup BUG at lib/string_helpers.c from scsi fusion mptsas

[Kees Cook <keescook@chromium.org>]

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@hansenpartnership.com> 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.

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