LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH] scsi: advansys: use struct_size() in kzalloc()
@ 2019-01-04 21:22 Gustavo A. R. Silva
  2019-01-11 15:46 ` Hannes Reinecke
  0 siblings, 1 reply; 5+ messages in thread
From: Gustavo A. R. Silva @ 2019-01-04 21:22 UTC (permalink / raw)
  To: Matthew Wilcox, Hannes Reinecke, James E.J. Bottomley,
	Martin K. Petersen
  Cc: linux-scsi, linux-kernel, Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the
size of a structure that has a zero-sized array at the end, along with memory
for some number of elements for that array. For example:

struct foo {
    int stuff;
    void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now
use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

This code was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/scsi/advansys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index d37584403c33..6c274e6e1c33 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -7576,8 +7576,8 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
 			return ASC_ERROR;
 		}
 
-		asc_sg_head = kzalloc(sizeof(asc_scsi_q->sg_head) +
-			use_sg * sizeof(struct asc_sg_list), GFP_ATOMIC);
+		asc_sg_head = kzalloc(struct_size(asc_sg_head, sg_list, use_sg),
+				      GFP_ATOMIC);
 		if (!asc_sg_head) {
 			scsi_dma_unmap(scp);
 			scp->result = HOST_BYTE(DID_SOFT_ERROR);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] scsi: advansys: use struct_size() in kzalloc()
  2019-01-04 21:22 [PATCH] scsi: advansys: use struct_size() in kzalloc() Gustavo A. R. Silva
@ 2019-01-11 15:46 ` Hannes Reinecke
  2019-01-11 16:41   ` James Bottomley
  0 siblings, 1 reply; 5+ messages in thread
From: Hannes Reinecke @ 2019-01-11 15:46 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Matthew Wilcox, Hannes Reinecke,
	James E.J. Bottomley, Martin K. Petersen
  Cc: linux-scsi, linux-kernel

On 1/4/19 10:22 PM, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding the
> size of a structure that has a zero-sized array at the end, along with memory
> for some number of elements for that array. For example:
> 
> struct foo {
>      int stuff;
>      void *entry[];
> };
> 
> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
> 
> Instead of leaving these open-coded and prone to type mistakes, we can now
> use the new struct_size() helper:
> 
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
> 
> This code was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>   drivers/scsi/advansys.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> index d37584403c33..6c274e6e1c33 100644
> --- a/drivers/scsi/advansys.c
> +++ b/drivers/scsi/advansys.c
> @@ -7576,8 +7576,8 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
>   			return ASC_ERROR;
>   		}
>   
> -		asc_sg_head = kzalloc(sizeof(asc_scsi_q->sg_head) +
> -			use_sg * sizeof(struct asc_sg_list), GFP_ATOMIC);
> +		asc_sg_head = kzalloc(struct_size(asc_sg_head, sg_list, use_sg),
> +				      GFP_ATOMIC);
>   		if (!asc_sg_head) {
>   			scsi_dma_unmap(scp);
>   			scp->result = HOST_BYTE(DID_SOFT_ERROR);
> 
If you want ...

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] scsi: advansys: use struct_size() in kzalloc()
  2019-01-11 15:46 ` Hannes Reinecke
@ 2019-01-11 16:41   ` James Bottomley
  2019-01-11 16:54     ` Matthew Wilcox
  0 siblings, 1 reply; 5+ messages in thread
From: James Bottomley @ 2019-01-11 16:41 UTC (permalink / raw)
  To: Hannes Reinecke, Gustavo A. R. Silva, Matthew Wilcox,
	Hannes Reinecke, Martin K. Petersen
  Cc: linux-scsi, linux-kernel

On Fri, 2019-01-11 at 16:46 +0100, Hannes Reinecke wrote:
> On 1/4/19 10:22 PM, Gustavo A. R. Silva wrote:
> > One of the more common cases of allocation size calculations is
> > finding the
> > size of a structure that has a zero-sized array at the end, along
> > with memory
> > for some number of elements for that array. For example:
> > 
> > struct foo {
> >      int stuff;
> >      void *entry[];
> > };
> > 
> > instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count,
> > GFP_KERNEL);
> > 
> > Instead of leaving these open-coded and prone to type mistakes, we
> > can now
> > use the new struct_size() helper:
> > 
> > instance = kzalloc(struct_size(instance, entry, count),
> > GFP_KERNEL);
> > 
> > This code was detected with the help of Coccinelle.
> > 
> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > ---
> >   drivers/scsi/advansys.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> > index d37584403c33..6c274e6e1c33 100644
> > --- a/drivers/scsi/advansys.c
> > +++ b/drivers/scsi/advansys.c
> > @@ -7576,8 +7576,8 @@ static int asc_build_req(struct asc_board
> > *boardp, struct scsi_cmnd *scp,
> >   			return ASC_ERROR;
> >   		}
> >   
> > -		asc_sg_head = kzalloc(sizeof(asc_scsi_q->sg_head)
> > +
> > -			use_sg * sizeof(struct asc_sg_list),
> > GFP_ATOMIC);
> > +		asc_sg_head = kzalloc(struct_size(asc_sg_head,
> > sg_list, use_sg),
> > +				      GFP_ATOMIC);
> >   		if (!asc_sg_head) {
> >   			scsi_dma_unmap(scp);
> >   			scp->result = HOST_BYTE(DID_SOFT_ERROR);
> > 
> 
> If you want ...

Are we sure there's a benefit to this?  It's obvious that the current
code is correct but no-one's likely to test the new code for quite some
time, so changing the code introduces risk. What's the benefit of
making the change in legacy drivers?  Just because we have a new, shiny
macro doesn't mean we have to force its use everywhere.

I would recommend we have a rational needs test: so run the coccinelle
script over all the drivers to find out where this construct is used,
but only update those that are actually buggy with the new macro.

James


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] scsi: advansys: use struct_size() in kzalloc()
  2019-01-11 16:41   ` James Bottomley
@ 2019-01-11 16:54     ` Matthew Wilcox
  2019-01-11 17:23       ` James Bottomley
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2019-01-11 16:54 UTC (permalink / raw)
  To: James Bottomley
  Cc: Hannes Reinecke, Gustavo A. R. Silva, Hannes Reinecke,
	Martin K. Petersen, linux-scsi, linux-kernel

On Fri, Jan 11, 2019 at 08:41:43AM -0800, James Bottomley wrote:
> On Fri, 2019-01-11 at 16:46 +0100, Hannes Reinecke wrote:
> > > -		asc_sg_head = kzalloc(sizeof(asc_scsi_q->sg_head)
> > > +
> > > -			use_sg * sizeof(struct asc_sg_list),
> > > GFP_ATOMIC);
> > > +		asc_sg_head = kzalloc(struct_size(asc_sg_head,
> > > sg_list, use_sg),
> > > +				      GFP_ATOMIC);
> > If you want ...
> 
> Are we sure there's a benefit to this?  It's obvious that the current
> code is correct but no-one's likely to test the new code for quite some
> time, so changing the code introduces risk. What's the benefit of
> making the change in legacy drivers?  Just because we have a new, shiny
> macro doesn't mean we have to force its use everywhere.
> 
> I would recommend we have a rational needs test: so run the coccinelle
> script over all the drivers to find out where this construct is used,
> but only update those that are actually buggy with the new macro.

It's hard to tell whether they're buggy.  The problem being defended
against here is integer overflow.  So can 'use_sg' ever get large enough
that sizeof(asc_scsi_q->sg_head) + use_sg * sizeof(struct asc_sg_list)
is larger than 4 billion?  Probably not; I imagine there's some
rational sane limit elsewhere that says "No more than 256 SG elements"
or something.

But I don't know without checking.  Is there some device-specific ioctl
where the user can specify 2^31 scatterlist entries and somebody forgot
to check?  This macro is a defense-in-depth strategy, so using it as
widely as possible makes more sense than arguing about whether there
are already adequate safeguards in place.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] scsi: advansys: use struct_size() in kzalloc()
  2019-01-11 16:54     ` Matthew Wilcox
@ 2019-01-11 17:23       ` James Bottomley
  0 siblings, 0 replies; 5+ messages in thread
From: James Bottomley @ 2019-01-11 17:23 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Hannes Reinecke, Gustavo A. R. Silva, Hannes Reinecke,
	Martin K. Petersen, linux-scsi, linux-kernel

On Fri, 2019-01-11 at 08:54 -0800, Matthew Wilcox wrote:
> On Fri, Jan 11, 2019 at 08:41:43AM -0800, James Bottomley wrote:
> > On Fri, 2019-01-11 at 16:46 +0100, Hannes Reinecke wrote:
> > > > -		asc_sg_head = kzalloc(sizeof(asc_scsi_q-
> > > > >sg_head)
> > > > +
> > > > -			use_sg * sizeof(struct asc_sg_list),
> > > > GFP_ATOMIC);
> > > > +		asc_sg_head = kzalloc(struct_size(asc_sg_head,
> > > > sg_list, use_sg),
> > > > +				      GFP_ATOMIC);
> > > 
> > > If you want ...
> > 
> > Are we sure there's a benefit to this?  It's obvious that the
> > current
> > code is correct but no-one's likely to test the new code for quite
> > some
> > time, so changing the code introduces risk. What's the benefit of
> > making the change in legacy drivers?  Just because we have a new,
> > shiny
> > macro doesn't mean we have to force its use everywhere.
> > 
> > I would recommend we have a rational needs test: so run the
> > coccinelle
> > script over all the drivers to find out where this construct is
> > used,
> > but only update those that are actually buggy with the new macro.
> 
> It's hard to tell whether they're buggy.  The problem being defended
> against here is integer overflow.  So can 'use_sg' ever get large
> enough that sizeof(asc_scsi_q->sg_head) + use_sg * sizeof(struct
> asc_sg_list) is larger than 4 billion?  Probably not; I imagine
> there's some rational sane limit elsewhere that says "No more than
> 256 SG elements" or something.

OK so firstly describing why we're doing this would have been
enormously useful.

Secondly, as you say, even with the enhanced rationale I'm not sure it
provides any benefit:  he advansys is two drivers squashed together:
the asc_ and adv_ prefixes.  It looks like the adv_ variant does check
the number of sg elements against the max, but asc_ doesn't; it relies
on the host limit sg_tablesize.  In both cases the actual limit is
somewhere around 255, so if the user can control the value they can
definitely cause corruption long before we get to mathematical
overflow.

The limit should be enforced by blk_queue_max_segments() and I think
this is done in all cases (including SG_IO).

> But I don't know without checking.  Is there some device-specific
> ioctl where the user can specify 2^31 scatterlist entries and
> somebody forgot to check?  This macro is a defense-in-depth strategy,
> so using it as widely as possible makes more sense than arguing about
> whether there are already adequate safeguards in place.

OK, so this is a question worth asking (I believe the answer to be "no"
but I could be wrong) because if there is some way of getting the value
over the driver internal table max (which is fixed for quite a few
drivers) we can induce corruption which this macro won't defend against
and if we need to, we should probably defend in sg_map_dma().

James



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-04 21:22 [PATCH] scsi: advansys: use struct_size() in kzalloc() Gustavo A. R. Silva
2019-01-11 15:46 ` Hannes Reinecke
2019-01-11 16:41   ` James Bottomley
2019-01-11 16:54     ` Matthew Wilcox
2019-01-11 17:23       ` James Bottomley

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox