| Message ID | 20170725195110.uwrzzkzvrbfqv7ld@mwanda (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Tue, 2017-07-25 at 22:51 +0300, Dan Carpenter wrote: > We're putting a NUL terminator one character beyond the end of the > struct and that's obviously wrong. On the other hand, I'm not positive > this is the correct fix. This change was added deliberately and was > mentioned in the changlog of commit b836439faf04 ("aacraid: 4KB sector > support"). The relevant section is "Also fix up a name truncation > problem". Can someone review this code and figure out the right thing > to do? > > Fixes: b836439faf04 ("aacraid: 4KB sector support") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c > index 4591113c49de..22c7461f65c9 100644 > --- a/drivers/scsi/aacraid/aachba.c > +++ b/drivers/scsi/aacraid/aachba.c > @@ -549,7 +549,7 @@ static void get_container_name_callback(void *context, struct fib * fibptr) > if ((le32_to_cpu(get_name_reply->status) == CT_OK) > && (get_name_reply->data[0] != '\0')) { > char *sp = get_name_reply->data; > - sp[sizeof(((struct aac_get_name_resp *)NULL)->data)] = '\0'; > + sp[sizeof(((struct aac_get_name_resp *)NULL)->data) - 1] = '\0'; > while (*sp == ' ') > ++sp; > if (*sp) { Hello Dan, If others agree with the approach of this patch, please use FIELD_SIZEOF() instead of leaving it open-coded. Thanks, Bart.
Dan, > We're putting a NUL terminator one character beyond the end of the > struct and that's obviously wrong. On the other hand, I'm not positive > this is the correct fix. This change was added deliberately and was > mentioned in the changlog of commit b836439faf04 ("aacraid: 4KB sector > support"). The relevant section is "Also fix up a name truncation > problem". Can someone review this code and figure out the right thing > to do? I guess that's a feeble attempt to compensate for the fact it's not a C string. The string coming from the controller firmware appears to be a fixed 16-byte length. And so is the inquiry buffer that it's being copied to. If the code would just use the inquiry string verbatim instead of removing leading spaces and padding it. But there was probably some crappy device out there that broke something for someone... Anyway. Terminating the string is not the right fix.
It would be simple enough to write it like you say, but it probably should be done by someone who is able to test it. regards, dan carpenter
Dan, > It would be simple enough to write it like you say, but it probably > should be done by someone who is able to test it. Indeed, I don't have any aacraid hw. Raghava, could you please take a look at this issue? https://patchwork.kernel.org/patch/9863105/ Thank you!
> > Dan, > > > We're putting a NUL terminator one character beyond the end of the > > struct and that's obviously wrong. On the other hand, I'm not > > positive this is the correct fix. This change was added deliberately > > and was mentioned in the changlog of commit b836439faf04 ("aacraid: > > 4KB sector support"). The relevant section is "Also fix up a name > > truncation problem". Can someone review this code and figure out the > > right thing to do? > > I guess that's a feeble attempt to compensate for the fact it's not a C string. The > string coming from the controller firmware appears to be a fixed 16-byte length. > And so is the inquiry buffer that it's being copied to. > > If the code would just use the inquiry string verbatim instead of removing > leading spaces and padding it. But there was probably some crappy device out > there that broke something for someone... Hi Martin, Dan, The issue is that we are making an inquiry response from container/RAID info. We could also have included a "pad" byte to terminate the string, as the fib data is 512 bytes. My assumption is that somehow back in the day, someone managed to get odd characters into the name. Terminating the string early would truncate the name ... -Dave > > Anyway. Terminating the string is not the right fix. > > -- > Martin K. Petersen Oracle Linux Engineering
Dave, > The issue is that we are making an inquiry response from > container/RAID info. We could also have included a "pad" byte to > terminate the string, as the fib data is 512 bytes. My assumption is > that somehow back in the day, someone managed to get odd characters > into the name. > > Terminating the string early would truncate the name ... So what do you propose as a fix?
> Dave, > > > The issue is that we are making an inquiry response from > > container/RAID info. We could also have included a "pad" byte to > > terminate the string, as the fib data is 512 bytes. My assumption is > > that somehow back in the day, someone managed to get odd characters > > into the name. > > > > Terminating the string early would truncate the name ... > > So what do you propose as a fix? We can add the pad byte to the struct, and terminate the string there. I'm out the office now, but Raghava or I will send up a patch ... That way, folks won't need to contemplate what appears to be a blatant out of bounds reference. -Dave > > -- > Martin K. Petersen Oracle Linux Engineering
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c index 4591113c49de..22c7461f65c9 100644 --- a/drivers/scsi/aacraid/aachba.c +++ b/drivers/scsi/aacraid/aachba.c @@ -549,7 +549,7 @@ static void get_container_name_callback(void *context, struct fib * fibptr) if ((le32_to_cpu(get_name_reply->status) == CT_OK) && (get_name_reply->data[0] != '\0')) { char *sp = get_name_reply->data; - sp[sizeof(((struct aac_get_name_resp *)NULL)->data)] = '\0'; + sp[sizeof(((struct aac_get_name_resp *)NULL)->data) - 1] = '\0'; while (*sp == ' ') ++sp; if (*sp) {
We're putting a NUL terminator one character beyond the end of the struct and that's obviously wrong. On the other hand, I'm not positive this is the correct fix. This change was added deliberately and was mentioned in the changlog of commit b836439faf04 ("aacraid: 4KB sector support"). The relevant section is "Also fix up a name truncation problem". Can someone review this code and figure out the right thing to do? Fixes: b836439faf04 ("aacraid: 4KB sector support") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>