[PATCH] SCSI: aacraid: potential integer overflow in aac_get_containers()

[PATCH] SCSI: aacraid: potential integer overflow in aac_get_containers()

[Haogang Chen]

There is a potential integer overflow in aac_get_containers(). When
maximum_num_containers is large, the subsequent call to kzalloc() will
allocate a buffer smaller than expected, which leads to memory
corruption in the for loop.

The patch replaces kzalloc with kcalloc.

Signed-off-by: Haogang Chen <haogangchen@gmail.com>
---
 drivers/scsi/aacraid/aachba.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 409f580..440b84d 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -381,7 +381,7 @@ int aac_get_containers(struct aac_dev *dev)
 
 	if (maximum_num_containers < MAXIMUM_NUM_CONTAINERS)
 		maximum_num_containers = MAXIMUM_NUM_CONTAINERS;
-	fsa_dev_ptr = kzalloc(sizeof(*fsa_dev_ptr) * maximum_num_containers,
+	fsa_dev_ptr = kcalloc(maximum_num_containers, sizeof(*fsa_dev_ptr), 
 			GFP_KERNEL);
 	if (!fsa_dev_ptr)
 		return -ENOMEM;
-- 
1.7.5.4

RE: [PATCH] SCSI: aacraid: potential integer overflow in aac_get_containers()

[Achim Leubner]

Acked-by: Achim Leubner <Achim_Leubner@pmc-sierra.com>


-----Original Message-----
From: linux-scsi-owner@vger.kernel.org
[mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Haogang Chen
Sent: Donnerstag, 1. Dezember 2011 03:30
To: aacraid@adaptec.com
Cc: JBottomley@parallels.com; linux-scsi@vger.kernel.org;
linux-kernel@vger.kernel.org; haogangchen@gmail.com
Subject: [PATCH] SCSI: aacraid: potential integer overflow in
aac_get_containers()

There is a potential integer overflow in aac_get_containers(). When
maximum_num_containers is large, the subsequent call to kzalloc() will
allocate a buffer smaller than expected, which leads to memory
corruption in the for loop.

The patch replaces kzalloc with kcalloc.

Signed-off-by: Haogang Chen <haogangchen@gmail.com>
---
 drivers/scsi/aacraid/aachba.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c
b/drivers/scsi/aacraid/aachba.c
index 409f580..440b84d 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -381,7 +381,7 @@ int aac_get_containers(struct aac_dev *dev)
 
 	if (maximum_num_containers < MAXIMUM_NUM_CONTAINERS)
 		maximum_num_containers = MAXIMUM_NUM_CONTAINERS;
-	fsa_dev_ptr = kzalloc(sizeof(*fsa_dev_ptr) *
maximum_num_containers,
+	fsa_dev_ptr = kcalloc(maximum_num_containers,
sizeof(*fsa_dev_ptr), 
 			GFP_KERNEL);
 	if (!fsa_dev_ptr)
 		return -ENOMEM;
-- 
1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] SCSI: aacraid: potential integer overflow in aac_get_containers()

[Mark Salyzyn]

NAK

I dispute that it is necessary or worth the additional abstraction,
since MAXIMUM_NUM_CONTAINERS is in the order of 32, and
sizeof(*fsa_dev_ptr) is in the range of 80 bytes ... There is a LONG
road to hoe to get to the point of overload!

kcalloc -> __kmalloc(size_t, flags | __GFP_ZERO);
kzalloc -> kmalloc(size_t, flags | __GFP_ZERO);
kmalloc -> __kmalloc(size_t, flags);

kcalloc can not allocate a larger entity than kzalloc. But alas it can
report that the size has exceeded ULONG_MAX so that part of the patch is
VERY sound.

Sincerely -- Mark Salyzyn

-----Original Message-----
From: linux-scsi-owner@vger.kernel.org
[mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Haogang Chen
Sent: Wednesday, November 30, 2011 9:30 PM
To: aacraid@adaptec.com
Cc: JBottomley@parallels.com; linux-scsi@vger.kernel.org;
linux-kernel@vger.kernel.org; haogangchen@gmail.com
Subject: [PATCH] SCSI: aacraid: potential integer overflow in
aac_get_containers()

There is a potential integer overflow in aac_get_containers(). When
maximum_num_containers is large, the subsequent call to kzalloc() will
allocate a buffer smaller than expected, which leads to memory
corruption in the for loop.

The patch replaces kzalloc with kcalloc.

Signed-off-by: Haogang Chen <haogangchen@gmail.com>
---
 drivers/scsi/aacraid/aachba.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c
b/drivers/scsi/aacraid/aachba.c
index 409f580..440b84d 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -381,7 +381,7 @@ int aac_get_containers(struct aac_dev *dev)
 
 	if (maximum_num_containers < MAXIMUM_NUM_CONTAINERS)
 		maximum_num_containers = MAXIMUM_NUM_CONTAINERS;
-	fsa_dev_ptr = kzalloc(sizeof(*fsa_dev_ptr) *
maximum_num_containers,
+	fsa_dev_ptr = kcalloc(maximum_num_containers,
sizeof(*fsa_dev_ptr), 
 			GFP_KERNEL);
 	if (!fsa_dev_ptr)
 		return -ENOMEM;
-- 
1.7.5.4

Re: [PATCH] SCSI: aacraid: potential integer overflow in aac_get_containers()

[Haogang Chen]

No, maximum_num_containers is not bounded to MAXIMUM_NUM_CONTAINERS,
it coud be very large. If you look at the code carefully:

if (maximum_num_containers < MAXIMUM_NUM_CONTAINERS)
        maximum_num_containers = MAXIMUM_NUM_CONTAINERS;

MAXIMUM_NUM_CONTAINERS (32) is actually the *minimal* value of
maximum_num_containers, though I'm not sure what's the underlying
logic of this check.


- Haogang



On Fri, Dec 2, 2011 at 11:03 AM, Mark Salyzyn
<mark_salyzyn@us.xyratex.com> wrote:
> NAK
>
> I dispute that it is necessary or worth the additional abstraction,
> since MAXIMUM_NUM_CONTAINERS is in the order of 32, and
> sizeof(*fsa_dev_ptr) is in the range of 80 bytes ... There is a LONG
> road to hoe to get to the point of overload!
>
> kcalloc -> __kmalloc(size_t, flags | __GFP_ZERO);
> kzalloc -> kmalloc(size_t, flags | __GFP_ZERO);
> kmalloc -> __kmalloc(size_t, flags);
>
> kcalloc can not allocate a larger entity than kzalloc. But alas it can
> report that the size has exceeded ULONG_MAX so that part of the patch is
> VERY sound.
>
> Sincerely -- Mark Salyzyn
>
> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Haogang Chen
> Sent: Wednesday, November 30, 2011 9:30 PM
> To: aacraid@adaptec.com
> Cc: JBottomley@parallels.com; linux-scsi@vger.kernel.org;
> linux-kernel@vger.kernel.org; haogangchen@gmail.com
> Subject: [PATCH] SCSI: aacraid: potential integer overflow in
> aac_get_containers()
>
> There is a potential integer overflow in aac_get_containers(). When
> maximum_num_containers is large, the subsequent call to kzalloc() will
> allocate a buffer smaller than expected, which leads to memory
> corruption in the for loop.
>
> The patch replaces kzalloc with kcalloc.
>
> Signed-off-by: Haogang Chen <haogangchen@gmail.com>
> ---
>  drivers/scsi/aacraid/aachba.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/aacraid/aachba.c
> b/drivers/scsi/aacraid/aachba.c
> index 409f580..440b84d 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -381,7 +381,7 @@ int aac_get_containers(struct aac_dev *dev)
>
>        if (maximum_num_containers < MAXIMUM_NUM_CONTAINERS)
>                maximum_num_containers = MAXIMUM_NUM_CONTAINERS;
> -       fsa_dev_ptr = kzalloc(sizeof(*fsa_dev_ptr) *
> maximum_num_containers,
> +       fsa_dev_ptr = kcalloc(maximum_num_containers,
> sizeof(*fsa_dev_ptr),
>                        GFP_KERNEL);
>        if (!fsa_dev_ptr)
>                return -ENOMEM;
> --
> 1.7.5.4
> ______________________________________________________________________
> This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
>
> Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
>
> Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
>
> The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
> ______________________________________________________________________
>
>