diff mbox series

[v2,next] scsi: lpfc: Use struct_size() helper

Message ID ZG0fDdY/PPQ/ijlt@work
State New
Headers
Series [v2,next] scsi: lpfc: Use struct_size() helper |

Commit Message

Gustavo A. R. Silva May 23, 2023, 8:16 p.m. UTC 
  Prefer struct_size() over open-coded versions of idiom:

sizeof(struct-with-flex-array) + sizeof(typeof-flex-array-elements) * count

where count is the max number of items the flexible array is supposed to
contain.

Link: https://github.com/KSPP/linux/issues/160
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Use literal 1 in call to struct_size(), instead of rap->no_of_objects
   (Kees Cook). 

v1:
 - Link: https://lore.kernel.org/linux-hardening/99e06733f5f35c6cd62e05f530b93107bfd03362.1684358315.git.gustavoars@kernel.org/

 drivers/scsi/lpfc/lpfc_ct.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Justin Tee May 24, 2023, 5:25 a.m. UTC | #1 
no_of_objects may be hardcoded to 1 right now, but does it make more
sense to use?

struct_size(rap, obj, be32_to_cpu(rap->no_of_objects));

We probably should have declared no_of_objects as __be32 to have
avoided this confusion.

On Tue, May 23, 2023 at 1:33 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> Prefer struct_size() over open-coded versions of idiom:
>
> sizeof(struct-with-flex-array) + sizeof(typeof-flex-array-elements) * count
>
> where count is the max number of items the flexible array is supposed to
> contain.
>
> Link: https://github.com/KSPP/linux/issues/160
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Changes in v2:
>  - Use literal 1 in call to struct_size(), instead of rap->no_of_objects
>    (Kees Cook).
>
> v1:
>  - Link: https://lore.kernel.org/linux-hardening/99e06733f5f35c6cd62e05f530b93107bfd03362.1684358315.git.gustavoars@kernel.org/
>
>  drivers/scsi/lpfc/lpfc_ct.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c
> index e880d127d7f5..f52aeb73af8d 100644
> --- a/drivers/scsi/lpfc/lpfc_ct.c
> +++ b/drivers/scsi/lpfc/lpfc_ct.c
> @@ -3747,9 +3747,7 @@ lpfc_vmid_cmd(struct lpfc_vport *vport,
>                 rap->no_of_objects = cpu_to_be32(1);
>                 rap->obj[0].entity_id_len = vmid->vmid_len;
>                 memcpy(rap->obj[0].entity_id, vmid->host_vmid, vmid->vmid_len);
> -               size = RAPP_IDENT_OFFSET +
> -                       sizeof(struct lpfc_vmid_rapp_ident_list) +
> -                       sizeof(struct entity_id_object);
> +               size = RAPP_IDENT_OFFSET + struct_size(rap, obj, 1);
>                 retry = 1;
>                 break;
>
> @@ -3767,9 +3765,7 @@ lpfc_vmid_cmd(struct lpfc_vport *vport,
>                 dap->no_of_objects = cpu_to_be32(1);
>                 dap->obj[0].entity_id_len = vmid->vmid_len;
>                 memcpy(dap->obj[0].entity_id, vmid->host_vmid, vmid->vmid_len);
> -               size = DAPP_IDENT_OFFSET +
> -                       sizeof(struct lpfc_vmid_dapp_ident_list) +
> -                       sizeof(struct entity_id_object);
> +               size = DAPP_IDENT_OFFSET + struct_size(dap, obj, 1);
>                 write_lock(&vport->vmid_lock);
>                 vmid->flag &= ~LPFC_VMID_REGISTERED;
>                 write_unlock(&vport->vmid_lock);
> --
> 2.34.1
>
Kees Cook May 26, 2023, 5:41 p.m. UTC | #2 
On Tue, May 23, 2023 at 10:25:20PM -0700, Justin Tee wrote:
> no_of_objects may be hardcoded to 1 right now, but does it make more
> sense to use?
> 
> struct_size(rap, obj, be32_to_cpu(rap->no_of_objects));

Oh yeah, that's nicer. :)

> We probably should have declared no_of_objects as __be32 to have
> avoided this confusion.

Perhaps this patch can add that too?
Justin Tee May 26, 2023, 6:06 p.m. UTC | #3 
> > We probably should have declared no_of_objects as __be32 to have
> > avoided this confusion.
>
> Perhaps this patch can add that too?

Yes, please.  Something like this is fine:

diff --git a/drivers/scsi/lpfc/lpfc_hw.h b/drivers/scsi/lpfc/lpfc_hw.h
index 19b2d2754f32..e5a639d96122 100644
--- a/drivers/scsi/lpfc/lpfc_hw.h
+++ b/drivers/scsi/lpfc/lpfc_hw.h
@@ -1414,12 +1414,12 @@ struct app_id_object {
 };

 struct lpfc_vmid_rapp_ident_list {
-       uint32_t no_of_objects;
+       __be32 no_of_objects;
        struct entity_id_object obj[1];
 };

 struct lpfc_vmid_dapp_ident_list {
-       uint32_t no_of_objects;
+       __be32 no_of_objects;
        struct entity_id_object obj[1];
 };
Martin K. Petersen May 31, 2023, 3:26 p.m. UTC | #4 
Gustavo/Justin,

>> > We probably should have declared no_of_objects as __be32 to have
>> > avoided this confusion.
>>
>> Perhaps this patch can add that too?
>
> Yes, please.  Something like this is fine:

Please send a formal patch. Thanks!
Justin Tee May 31, 2023, 9:59 p.m. UTC | #5 
Hi Martin,

Sure, will do.  I'll post the formal patch shortly.

The __be32 no_of_objects comment can be ignored for this patch as I've
already incorporated that into another patch with subject line: [PATCH
1/1] lpfc: Fix incorrect big endian type assignments in FDMI and VMID
paths

Thanks,
Justin


On Wed, May 31, 2023 at 8:27 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Gustavo/Justin,
>
> >> > We probably should have declared no_of_objects as __be32 to have
> >> > avoided this confusion.
> >>
> >> Perhaps this patch can add that too?
> >
> > Yes, please.  Something like this is fine:
>
> Please send a formal patch. Thanks!
>
> --
> Martin K. Petersen      Oracle Linux Engineering
diff mbox series

Patch

diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c
index e880d127d7f5..f52aeb73af8d 100644
--- a/drivers/scsi/lpfc/lpfc_ct.c
+++ b/drivers/scsi/lpfc/lpfc_ct.c
@@ -3747,9 +3747,7 @@  lpfc_vmid_cmd(struct lpfc_vport *vport,
 		rap->no_of_objects = cpu_to_be32(1);
 		rap->obj[0].entity_id_len = vmid->vmid_len;
 		memcpy(rap->obj[0].entity_id, vmid->host_vmid, vmid->vmid_len);
-		size = RAPP_IDENT_OFFSET +
-			sizeof(struct lpfc_vmid_rapp_ident_list) +
-			sizeof(struct entity_id_object);
+		size = RAPP_IDENT_OFFSET + struct_size(rap, obj, 1);
 		retry = 1;
 		break;
 
@@ -3767,9 +3765,7 @@  lpfc_vmid_cmd(struct lpfc_vport *vport,
 		dap->no_of_objects = cpu_to_be32(1);
 		dap->obj[0].entity_id_len = vmid->vmid_len;
 		memcpy(dap->obj[0].entity_id, vmid->host_vmid, vmid->vmid_len);
-		size = DAPP_IDENT_OFFSET +
-			sizeof(struct lpfc_vmid_dapp_ident_list) +
-			sizeof(struct entity_id_object);
+		size = DAPP_IDENT_OFFSET + struct_size(dap, obj, 1);
 		write_lock(&vport->vmid_lock);
 		vmid->flag &= ~LPFC_VMID_REGISTERED;
 		write_unlock(&vport->vmid_lock);