| Message ID | 20240223-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v1-7-9cd3882f0700@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: replace deprecated strncpy | expand |
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... > 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? > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > 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, ','); > >
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@google.com> > > --- > > 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@chromium.org>
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, ',');
@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 as setup_buffer is declared with static storage duration in the top-level scope which should NUL-initialize the whole buffer. Signed-off-by: Justin Stitt <justinstitt@google.com> --- drivers/scsi/wd33c93.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)