[v3] scsi: csiostor: Use kcalloc() instead of kzalloc()

Commit Message

Erick Archer March 30, 2024, 4:17 p.m. UTC

Use 2-factor multiplication argument form kcalloc() instead
of kzalloc().

Also, it is preferred to use sizeof(*pointer) instead of
sizeof(type) due to the type of the variable can change and
one needs not change the former (unlike the latter).

Link: https://github.com/KSPP/linux/issues/162
Reviewed-by: Gustavo A. R. Silva <[e-mail]>
Signed-off-by: Erick Archer <[e-mail]>
---
Changes in v2:
- Update the changelog text describing the sizeof()
  changes (Gustavo A. R. Silva)

Changes in v3:
- Add the "Reviewed-by:" tag.
- Rebase against linux-next.

Version 1:
Link: https://lore.kernel.org/linux-hardening/[e-mail]/

Version 2:
Link: https://lore.kernel.org/linux-hardening/[e-mail]/

Hi everyone,

This patch seems to be lost. Gustavo reviewed it on January 15, 2024
but the patch has not been applied since.

Thanks,
Erick
---
 drivers/scsi/csiostor/csio_init.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Comments

Erick Archer April 27, 2024, 1:52 p.m. UTC | #1

On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote:
> Use 2-factor multiplication argument form kcalloc() instead
> of kzalloc().
> 
> Also, it is preferred to use sizeof(*pointer) instead of
> sizeof(type) due to the type of the variable can change and
> one needs not change the former (unlike the latter).
> 
> Link: https://github.com/KSPP/linux/issues/162
> Reviewed-by: Gustavo A. R. Silva <[e-mail]>
> Signed-off-by: Erick Archer <[e-mail]>

Friendly ping. Any comments?

Regards,
Erick

Kees Cook April 29, 2024, 5:20 p.m. UTC | #2

On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote:
> Use 2-factor multiplication argument form kcalloc() instead
> of kzalloc().
> 
> Also, it is preferred to use sizeof(*pointer) instead of
> sizeof(type) due to the type of the variable can change and
> one needs not change the former (unlike the latter).
> 
> Link: https://github.com/KSPP/linux/issues/162
> Reviewed-by: Gustavo A. R. Silva <[e-mail]>
> Signed-off-by: Erick Archer <[e-mail]>
> ---
> Changes in v2:
> - Update the changelog text describing the sizeof()
>   changes (Gustavo A. R. Silva)
> 
> Changes in v3:
> - Add the "Reviewed-by:" tag.
> - Rebase against linux-next.
> 
> Version 1:
> Link: https://lore.kernel.org/linux-hardening/[e-mail]/
> 
> Version 2:
> Link: https://lore.kernel.org/linux-hardening/[e-mail]/
> 
> Hi everyone,
> 
> This patch seems to be lost. Gustavo reviewed it on January 15, 2024
> but the patch has not been applied since.

This looks correct to me. I can pick this up if no one else snags it?

Reviewed-by: Kees Cook <[e-mail]>

-Kees

> 
> Thanks,
> Erick
> ---
>  drivers/scsi/csiostor/csio_init.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c
> index d649b7a2a879..d72892e44fd1 100644
> --- a/drivers/scsi/csiostor/csio_init.c
> +++ b/drivers/scsi/csiostor/csio_init.c
> @@ -698,8 +698,7 @@ csio_lnodes_block_request(struct csio_hw *hw)
>  	struct csio_lnode **lnode_list;
>  	int cur_cnt = 0, ii;
>  
> -	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
> -			GFP_KERNEL);
> +	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
>  	if (!lnode_list) {
>  		csio_err(hw, "Failed to allocate lnodes_list");
>  		return;
> @@ -737,8 +736,7 @@ csio_lnodes_unblock_request(struct csio_hw *hw)
>  	struct csio_lnode **lnode_list;
>  	int cur_cnt = 0, ii;
>  
> -	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
> -			GFP_KERNEL);
> +	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
>  	if (!lnode_list) {
>  		csio_err(hw, "Failed to allocate lnodes_list");
>  		return;
> @@ -775,8 +773,7 @@ csio_lnodes_block_by_port(struct csio_hw *hw, uint8_t portid)
>  	struct csio_lnode **lnode_list;
>  	int cur_cnt = 0, ii;
>  
> -	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
> -			GFP_KERNEL);
> +	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
>  	if (!lnode_list) {
>  		csio_err(hw, "Failed to allocate lnodes_list");
>  		return;
> @@ -816,8 +813,7 @@ csio_lnodes_unblock_by_port(struct csio_hw *hw, uint8_t portid)
>  	struct csio_lnode **lnode_list;
>  	int cur_cnt = 0, ii;
>  
> -	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
> -			GFP_KERNEL);
> +	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
>  	if (!lnode_list) {
>  		csio_err(hw, "Failed to allocate lnodes_list");
>  		return;
> @@ -855,8 +851,7 @@ csio_lnodes_exit(struct csio_hw *hw, bool npiv)
>  	struct csio_lnode **lnode_list;
>  	int cur_cnt = 0, ii;
>  
> -	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
> -			GFP_KERNEL);
> +	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
>  	if (!lnode_list) {
>  		csio_err(hw, "lnodes_exit: Failed to allocate lnodes_list.\n");
>  		return;
> -- 
> 2.25.1
>

Martin K. Petersen April 29, 2024, 6:31 p.m. UTC | #3

Kees,

>> This patch seems to be lost. Gustavo reviewed it on January 15, 2024
>> but the patch has not been applied since.
>
> This looks correct to me. I can pick this up if no one else snags it?

I guess my original reply didn't make it out, I don't see it in the
archives.

My objections were:

 1. The original code is more readable to me than the proposed
    replacement.

 2. The original code has worked since introduced in 2012. Nobody has
    touched it since, presumably it's fine.

 3. I don't have the hardware and thus no way of validating the proposed
    changes.

So what is the benefit of me accepting this patch? We have had several
regressions in these conversions. Had one just last week, almost
identical in nature to the one at hand.

I am all for fixing code which is undergoing active use and development.
But I really don't see the benefit of updating a legacy driver which
hasn't seen updates in ages. Why risk introducing a regression?

Kees Cook April 29, 2024, 8:13 p.m. UTC | #4

On Mon, Apr 29, 2024 at 02:31:19PM -0400, Martin K. Petersen wrote:
> 
> Kees,
> 
> >> This patch seems to be lost. Gustavo reviewed it on January 15, 2024
> >> but the patch has not been applied since.
> >
> > This looks correct to me. I can pick this up if no one else snags it?
> 
> I guess my original reply didn't make it out, I don't see it in the
> archives.
> 
> My objections were:
> 
>  1. The original code is more readable to me than the proposed
>     replacement.

I guess this is a style preference. I find the proposed easier to read.
It also removes lines while doing it. :)

>  2. The original code has worked since introduced in 2012. Nobody has
>     touched it since, presumably it's fine.

The code itself is fine unless you have a 32-bit system with a malicious
card, so yeah, near zero risk.

>  3. I don't have the hardware and thus no way of validating the proposed
>     changes.

This is kind of an ongoing tension we have between driver code and
refactoring efforts. And this isn't a case where we can show identical
binary output, since this actively adds overflow checking via kcalloc()
internals.

> So what is the benefit of me accepting this patch? We have had several
> regressions in these conversions. Had one just last week, almost
> identical in nature to the one at hand.

People are working through large piles of known "weak code patterns"
with the goal of reaching 0 instances in the kernel. Usually this is for
ongoing greater compiler flag coverage, but this particular one is
harder for the compiler to warn on, so it's from Coccinelle patterns.

> I am all for fixing code which is undergoing active use and development.
> But I really don't see the benefit of updating a legacy driver which
> hasn't seen updates in ages. Why risk introducing a regression?

I see a common pattern where "why risk introducing a regression?" gets
paired with "we can't test this code". I'm really not sure what to do
about this given how much the kernel is changing all the time.

In this particular case, I guess all I can say is that it is a trivially
correct change that uses a more robust API and more idiomatic allocation
sizeof()s (i.e. use the sizeof() of what is being allocated, not a
potentially disconnected struct name).

-Kees

Finn Thain April 30, 2024, 1:54 a.m. UTC | #5

On Mon, 29 Apr 2024, Kees Cook wrote:

> this isn't a case where we can show identical binary output, since this 
> actively adds overflow checking via kcalloc() internals.
> 
> ...
> 
> it is a trivially correct change that uses a more robust API and more 
> idiomatic allocation sizeof()s

If a change is "trivially correct" then the proof is trivial too.

Based only on what you wrote above, omitting the overflow check would give 
binary equivalence. That validates the driver change (for hardware you 
lack).

But, since a build without the overflow check must contain a second 
change, you must validate that change too by showing that kcalloc() 
internals still work for every other caller. (You do this using hardware 
you have.)

James Bottomley May 1, 2024, 2:39 p.m. UTC | #6

On Mon, 2024-04-29 at 13:13 -0700, Kees Cook wrote:
> On Mon, Apr 29, 2024 at 02:31:19PM -0400, Martin K. Petersen wrote:
> > 
> > Kees,
> > 
> > > > This patch seems to be lost. Gustavo reviewed it on January 15,
> > > > 2024 but the patch has not been applied since.
> > > 
> > > This looks correct to me. I can pick this up if no one else snags
> > > it?
> > 
> > I guess my original reply didn't make it out, I don't see it in the
> > archives.
> > 
> > My objections were:
> > 
> >  1. The original code is more readable to me than the proposed
> >     replacement.
> 
> I guess this is a style preference. I find the proposed easier to
> read. It also removes lines while doing it. :)
> 
> >  2. The original code has worked since introduced in 2012. Nobody
> > has touched it since, presumably it's fine.
> 
> The code itself is fine unless you have a 32-bit system with a
> malicious card, so yeah, near zero risk.

Well, no actually zero: we assume plugged in hardware to operate
correctly (had this argument in the driver hardening thread a while
ago), but in this particular case you'd have to have a card with a very
high number of ports, which would cause kernel allocations to fail long
before anything could introduce an overflow of sizeof(struct csio_lnode
*) * hw->num_lns.

> >  3. I don't have the hardware and thus no way of validating the
> > proposed changes.
> 
> This is kind of an ongoing tension we have between driver code and
> refactoring efforts.

That's because we keep having cockups where we accept so called "zero
risk" changes to older drivers only to have people with the hardware
turn up months to years later demanding to know why we broke it.

Security is about balancing risks and the risk here of a malicious
adversary crafting an attack based on a driver so few people use (and
given they'd have to come up with modified hardware) seems equally
zero.

>  And this isn't a case where we can show identical binary output,
> since this actively adds overflow checking via kcalloc() internals.

Overflow checking which is unnecessary as I showed above.

> > So what is the benefit of me accepting this patch? We have had
> > several regressions in these conversions. Had one just last week,
> > almost identical in nature to the one at hand.
> 
> People are working through large piles of known "weak code patterns"
> with the goal of reaching 0 instances in the kernel. Usually this is
> for ongoing greater compiler flag coverage, but this particular one
> is harder for the compiler to warn on, so it's from Coccinelle
> patterns.

We understand the problem and we're happy to investigate and then
explain why something like this can't be exploited, so what's the issue
with adding it to the exceptions list given that, as you said, it's
never going to be compiler detected?

> > I am all for fixing code which is undergoing active use and
> > development. But I really don't see the benefit of updating a
> > legacy driver which hasn't seen updates in ages. Why risk
> > introducing a regression?
> 
> I see a common pattern where "why risk introducing a regression?"
> gets paired with "we can't test this code". I'm really not sure what
> to do about this given how much the kernel is changing all the time.

Well, it's a balance of risks, but given that there's zero chance of
exploitation of the potential overflow, it would seem that balance lies
on the side of not risking the regression.  I think if you could
demonstrate you were fixing an exploitable bug (without needing
modified hardware) the balance would lie differently.

> In this particular case, I guess all I can say is that it is a
> trivially correct change that uses a more robust API and more
> idiomatic allocation sizeof()s (i.e. use the sizeof() of what is
> being allocated, not a potentially disconnected struct name).

Which is somewhat similar to the statement other people made about the
strncpy replacement which eventually turned out to cause a problem.

James

Finn Thain May 2, 2024, 12:47 a.m. UTC | #7

On Wed, 1 May 2024, James Bottomley wrote:

> > The code itself is fine unless you have a 32-bit system with a 
> > malicious card, so yeah, near zero risk.
> 
> Well, no actually zero: we assume plugged in hardware to operate 
> correctly (had this argument in the driver hardening thread a while 
> ago), but in this particular case you'd have to have a card with a very 
> high number of ports, which would cause kernel allocations to fail long 
> before anything could introduce an overflow of sizeof(struct csio_lnode
> *) * hw->num_lns.
> 

Then it should be safe to add an equivalent assertion. E.g. 
BUG_ON(hw->num_lns > X) where X was derived either from knowledge of the 
hardware or from some known-safe kalloc() limit. Though I wonder whether 
BUG_ON() is the best way to encode preconditions for the benfit of static 
checkers...

Erick Archer May 11, 2024, 11:18 a.m. UTC | #8

Hi Martin, Kees and Finn,

On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote:
> Use 2-factor multiplication argument form kcalloc() instead
> of kzalloc().
> 
> Also, it is preferred to use sizeof(*pointer) instead of
> sizeof(type) due to the type of the variable can change and
> one needs not change the former (unlike the latter).
> 
> Link: https://github.com/KSPP/linux/issues/162
> Reviewed-by: Gustavo A. R. Silva <[e-mail]>
> Signed-off-by: Erick Archer <[e-mail]>
> ---
> 
Thank you very much for the reviews and comments.

Regards,
Erick

Erick Archer May 11, 2024, 3:10 p.m. UTC | #9

Hi James,

On Sat, May 11, 2024 at 01:18:46PM +0200, Erick Archer wrote:
> Hi Martin, Kees and Finn,
> 
> On Sat, Mar 30, 2024 at 05:17:53PM +0100, Erick Archer wrote:
> > Use 2-factor multiplication argument form kcalloc() instead
> > of kzalloc().
> > 
> > Also, it is preferred to use sizeof(*pointer) instead of
> > sizeof(type) due to the type of the variable can change and
> > one needs not change the former (unlike the latter).
> > 
> > Link: https://github.com/KSPP/linux/issues/162
> > Reviewed-by: Gustavo A. R. Silva <[e-mail]>
> > Signed-off-by: Erick Archer <[e-mail]>
> > ---
> > 
> Thank you very much for the reviews and comments.

Also, thanks for the comments and clarifications.

Regards,
Erick

Patch

diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c
index d649b7a2a879..d72892e44fd1 100644
--- a/drivers/scsi/csiostor/csio_init.c
+++ b/drivers/scsi/csiostor/csio_init.c
@@ -698,8 +698,7 @@  csio_lnodes_block_request(struct csio_hw *hw)
 	struct csio_lnode **lnode_list;
 	int cur_cnt = 0, ii;
 
-	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
-			GFP_KERNEL);
+	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
 	if (!lnode_list) {
 		csio_err(hw, "Failed to allocate lnodes_list");
 		return;
@@ -737,8 +736,7 @@  csio_lnodes_unblock_request(struct csio_hw *hw)
 	struct csio_lnode **lnode_list;
 	int cur_cnt = 0, ii;
 
-	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
-			GFP_KERNEL);
+	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
 	if (!lnode_list) {
 		csio_err(hw, "Failed to allocate lnodes_list");
 		return;
@@ -775,8 +773,7 @@  csio_lnodes_block_by_port(struct csio_hw *hw, uint8_t portid)
 	struct csio_lnode **lnode_list;
 	int cur_cnt = 0, ii;
 
-	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
-			GFP_KERNEL);
+	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
 	if (!lnode_list) {
 		csio_err(hw, "Failed to allocate lnodes_list");
 		return;
@@ -816,8 +813,7 @@  csio_lnodes_unblock_by_port(struct csio_hw *hw, uint8_t portid)
 	struct csio_lnode **lnode_list;
 	int cur_cnt = 0, ii;
 
-	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
-			GFP_KERNEL);
+	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
 	if (!lnode_list) {
 		csio_err(hw, "Failed to allocate lnodes_list");
 		return;
@@ -855,8 +851,7 @@  csio_lnodes_exit(struct csio_hw *hw, bool npiv)
 	struct csio_lnode **lnode_list;
 	int cur_cnt = 0, ii;
 
-	lnode_list = kzalloc((sizeof(struct csio_lnode *) * hw->num_lns),
-			GFP_KERNEL);
+	lnode_list = kcalloc(hw->num_lns, sizeof(*lnode_list), GFP_KERNEL);
 	if (!lnode_list) {
 		csio_err(hw, "lnodes_exit: Failed to allocate lnodes_list.\n");
 		return;