On Sat, Feb 24, 2024 at 10:44:12AM +1100, Finn Thain wrote:
>
> On Fri, 23 Feb 2024, Justin Stitt wrote:
>
> > @p1 is assigned to @setup_buffer and then we manually assign a NUL-byte
> > at the first index. This renders the following strlen() call useless.
> > Moreover, we don't need to reassign p1 to setup_buffer for any reason --
> > neither do we need to manually set a NUL-byte at the end. strscpy()
> > resolves all this code making it easier to read.
> >
> > Even considering the path where @str is falsey, the manual NUL-byte
> > assignment is useless
>
> And yet your patch would only remove one of those assignments...
The first is needed in case it is called again.
>
> > as setup_buffer is declared with static storage
> > duration in the top-level scope which should NUL-initialize the whole
> > buffer.
> >
>
> So, in order to review this patch, to try to avoid regressions, I would
> have to check your assumption that setup_buffer cannot change after being
> statically initialized. (The author of this code apparently was not
> willing to make that assumption.) It seems that patch review would require
> exhaustively searching for functions using the buffer, and examining the
> call graphs involving those functions. Is it really worth the effort?
It seems to be run for each device? Regardless, I think leaving the
initial "*p1 = '\0';" solves this. (Though I fear for parallel
initializations, but that was already buggy: this code is from pre-git
history...)
>
> > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
> > ---
> > drivers/scsi/wd33c93.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
> > index e4fafc77bd20..a44b60c9004a 100644
> > --- a/drivers/scsi/wd33c93.c
> > +++ b/drivers/scsi/wd33c93.c
> > @@ -1721,9 +1721,7 @@ wd33c93_setup(char *str)
> > p1 = setup_buffer;
> > *p1 = '\0';
> > if (str)
> > - strncpy(p1, str, SETUP_BUFFER_SIZE - strlen(setup_buffer));
> > - setup_buffer[SETUP_BUFFER_SIZE - 1] = '\0';
> > - p1 = setup_buffer;
> > + strscpy(p1, str, SETUP_BUFFER_SIZE);
> > i = 0;
> > while (*p1 && (i < MAX_SETUP_ARGS)) {
> > p2 = strchr(p1, ',');
> >
> >
I think this conversion looks right.
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
--
Kees Cook
