scsi: advansys: use struct_size() in kzalloc()
Subsystems (2)
PATCH 1/1 scsi: advansys: use struct_size() in kzalloc()
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.
--- drivers/scsi/advansys.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/advansys.c +2 -2
@@ -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 );
Signature · 1 line
2.20.1
Quoted 29 lines · 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 <[email protected]>--- drivers/scsi/advansys.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/scsi/advansys.c +0
-0
drivers/scsi/advansys.c static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp, · line 7583
Quoted 10 lines
drivers/scsi/advansys.c +2
-2@@ -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 ...
Cheers,
Hannes
Quoted 38 lines · Hannes Reinecke
Quoted 37 lines · 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 newstruct_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 <[email protected]>--- drivers/scsi/advansys.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/scsi/advansys.c +0
-0@@ -7576,8 +7576,8 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp, return ASC_ERROR; }
Quoted 13 lines
drivers/scsi/advansys.c static int asc_build_req(struct asc_board
Quoted 11 lines
drivers/scsi/advansys.c +3
-2- 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
Matthew Wilcox
maintainerQuoted 16 lines · James Bottomley
Quoted 8 lines · Hannes Reinecke
drivers/scsi/advansys.c static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp, · line 7580
Quoted 7 lines
(quoted diff) +1
-2- asc_sg_head = kzalloc(sizeof(asc_scsi_q->sg_head) + - use_sg * sizeof(struct asc_sg_list),
GFP_ATOMIC);(quoted diff) +1
-0+ asc_sg_head = kzalloc(struct_size(asc_sg_head,
sg_list,use_sg),drivers/scsi/advansys.c +1
-0+ 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.
Quoted 3 lines
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.
Quoted 28 lines · Matthew Wilcox
Quoted 21 lines · James Bottomley
Quoted 10 lines · Hannes Reinecke
drivers/scsi/advansys.c static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp, · line 7580
Quoted 8 lines
(quoted diff) +0
-1- asc_sg_head = kzalloc(sizeof(asc_scsi_q-Quoted 1 line
sg_head)(quoted diff) +1
-1+ - use_sg * sizeof(struct asc_sg_list),
GFP_ATOMIC);(quoted diff) +1
-0+ asc_sg_head = kzalloc(struct_size(asc_sg_head,
sg_list,use_sg),drivers/scsi/advansys.c +1
-0+ 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.Quoted 5 lines
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.
Quoted 6 lines
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 thatsizeof(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).
Quoted 5 lines
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