[PATCH 1/2] scsi: aacraid: reading out of bounds

[PATCH 1/2] scsi: aacraid: reading out of bounds

[Dan Carpenter]

"qd.id" comes directly from the copy_from_user() on the line before so
we should verify that it's within bounds.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This bug predates git.

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 707ee2f5954d..4591113c49de 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -3198,10 +3198,11 @@ static int query_disk(struct aac_dev *dev, void __user *arg)
 		return -EBUSY;
 	if (copy_from_user(&qd, arg, sizeof (struct aac_query_disk)))
 		return -EFAULT;
-	if (qd.cnum == -1)
+	if (qd.cnum == -1) {
+		if (qd.id < 0 || qd.id >= dev->maximum_num_containers)
+			return -EINVAL;
 		qd.cnum = qd.id;
-	else if ((qd.bus == -1) && (qd.id == -1) && (qd.lun == -1))
-	{
+	} else if ((qd.bus == -1) && (qd.id == -1) && (qd.lun == -1)) {
 		if (qd.cnum < 0 || qd.cnum >= dev->maximum_num_containers)
 			return -EINVAL;
 		qd.instance = dev->scsi_host_ptr->host_no;

[PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Dan Carpenter]

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) {

Re: [PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Bart Van Assche]

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.

Re: [PATCH 1/2] scsi: aacraid: reading out of bounds

[Martin K. Petersen]

Dan,

> "qd.id" comes directly from the copy_from_user() on the line before so
> we should verify that it's within bounds.

Applied to 4.13/scsi-fixes. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Martin K. Petersen]

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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Dan Carpenter]

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

Re: [PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Martin K. Petersen]

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!

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 1/2] scsi: aacraid: reading out of bounds

[Dave Carroll]

> 
> Dan,
> 
> > "qd.id" comes directly from the copy_from_user() on the line before so
> > we should verify that it's within bounds.
> 
> Applied to 4.13/scsi-fixes. Thanks!
> 
> --
> Martin K. Petersen      Oracle Linux Engineering

Acked-by: Dave Carroll <david.carroll@microsemi.com>

RE: [PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Dave Carroll]

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

Re: [PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Martin K. Petersen]

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?

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Dave Carroll]

> 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