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.
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
