scsi: 3w-sas: Replace 1-element arrays with flexible array members

Message ID 20230105004757.never.017-kees@kernel.org
State New
Headers
Series scsi: 3w-sas: Replace 1-element arrays with flexible array members |

Commit Message

Kees Cook Jan. 5, 2023, 12:48 a.m. UTC
  One-element arrays (and multi-element arrays being treated as
dynamically sized) are deprecated[1] and are being replaced with
flexible array members in support of the ongoing efforts to tighten the
FORTIFY_SOURCE routines on memcpy(), correctly instrument array indexing
with UBSAN_BOUNDS, and to globally enable -fstrict-flex-arrays=3.

Replace one-element arrays with flexible-array member in TW_Ioctl_Buf_Apache
and TW_Param_Apache, adjusting the explicit sizing calculations at the
same time.

This results in no differences in binary output.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays

Cc: Adam Radford <aradford@gmail.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/3w-sas.c | 12 ++++++------
 drivers/scsi/3w-sas.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)
  

Comments

Gustavo A. R. Silva Jan. 5, 2023, 5:47 p.m. UTC | #1
On Wed, Jan 04, 2023 at 04:48:01PM -0800, Kees Cook wrote:
> One-element arrays (and multi-element arrays being treated as
> dynamically sized) are deprecated[1] and are being replaced with
> flexible array members in support of the ongoing efforts to tighten the
> FORTIFY_SOURCE routines on memcpy(), correctly instrument array indexing
> with UBSAN_BOUNDS, and to globally enable -fstrict-flex-arrays=3.
> 
> Replace one-element arrays with flexible-array member in TW_Ioctl_Buf_Apache
> and TW_Param_Apache, adjusting the explicit sizing calculations at the
> same time.
> 
> This results in no differences in binary output.
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Cc: Adam Radford <aradford@gmail.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Cc: linux-scsi@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

This is great. :) It's quite similar to this other commit:

	44c5027bb5c8 ("scsi: 3w-9xxx: Use flexible array members to avoid struct padding")

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
--
Gustavo

> ---
>  drivers/scsi/3w-sas.c | 12 ++++++------
>  drivers/scsi/3w-sas.h |  4 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
> index 3ebe66151dcb..f41c93454f0c 100644
> --- a/drivers/scsi/3w-sas.c
> +++ b/drivers/scsi/3w-sas.c
> @@ -690,7 +690,7 @@ static void twl_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
>  		newcommand->request_id__lunl =
>  			cpu_to_le16(TW_REQ_LUN_IN(TW_LUN_OUT(newcommand->request_id__lunl), request_id));
>  		if (length) {
> -			newcommand->sg_list[0].address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache) - 1);
> +			newcommand->sg_list[0].address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
>  			newcommand->sg_list[0].length = TW_CPU_TO_SGL(length);
>  		}
>  		newcommand->sgl_entries__lunh =
> @@ -702,7 +702,7 @@ static void twl_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
>  		if (TW_SGL_OUT(oldcommand->opcode__sgloffset)) {
>  			/* Load the sg list */
>  			sgl = (TW_SG_Entry_ISO *)((u32 *)oldcommand+oldcommand->size - (sizeof(TW_SG_Entry_ISO)/4) + pae + (sizeof(dma_addr_t) > 4 ? 1 : 0));
> -			sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache) - 1);
> +			sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
>  			sgl->length = TW_CPU_TO_SGL(length);
>  			oldcommand->size += pae;
>  			oldcommand->size += sizeof(dma_addr_t) > 4 ? 1 : 0;
> @@ -748,7 +748,7 @@ static long twl_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
>  	data_buffer_length_adjusted = (driver_command.buffer_length + 511) & ~511;
>  
>  	/* Now allocate ioctl buf memory */
> -	cpu_addr = dma_alloc_coherent(&tw_dev->tw_pci_dev->dev, data_buffer_length_adjusted+sizeof(TW_Ioctl_Buf_Apache) - 1, &dma_handle, GFP_KERNEL);
> +	cpu_addr = dma_alloc_coherent(&tw_dev->tw_pci_dev->dev, data_buffer_length_adjusted + sizeof(TW_Ioctl_Buf_Apache), &dma_handle, GFP_KERNEL);
>  	if (!cpu_addr) {
>  		retval = -ENOMEM;
>  		goto out2;
> @@ -757,7 +757,7 @@ static long twl_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
>  	tw_ioctl = (TW_Ioctl_Buf_Apache *)cpu_addr;
>  
>  	/* Now copy down the entire ioctl */
> -	if (copy_from_user(tw_ioctl, argp, driver_command.buffer_length + sizeof(TW_Ioctl_Buf_Apache) - 1))
> +	if (copy_from_user(tw_ioctl, argp, driver_command.buffer_length + sizeof(TW_Ioctl_Buf_Apache)))
>  		goto out3;
>  
>  	/* See which ioctl we are doing */
> @@ -815,11 +815,11 @@ static long twl_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
>  	}
>  
>  	/* Now copy the entire response to userspace */
> -	if (copy_to_user(argp, tw_ioctl, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length - 1) == 0)
> +	if (copy_to_user(argp, tw_ioctl, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length) == 0)
>  		retval = 0;
>  out3:
>  	/* Now free ioctl buf memory */
> -	dma_free_coherent(&tw_dev->tw_pci_dev->dev, data_buffer_length_adjusted+sizeof(TW_Ioctl_Buf_Apache) - 1, cpu_addr, dma_handle);
> +	dma_free_coherent(&tw_dev->tw_pci_dev->dev, data_buffer_length_adjusted + sizeof(TW_Ioctl_Buf_Apache), cpu_addr, dma_handle);
>  out2:
>  	mutex_unlock(&tw_dev->ioctl_lock);
>  out:
> diff --git a/drivers/scsi/3w-sas.h b/drivers/scsi/3w-sas.h
> index b0508039a280..096dec29e2ac 100644
> --- a/drivers/scsi/3w-sas.h
> +++ b/drivers/scsi/3w-sas.h
> @@ -335,7 +335,7 @@ typedef struct TAG_TW_Ioctl_Apache {
>  	TW_Ioctl_Driver_Command driver_command;
>  	char padding[488];
>  	TW_Command_Full firmware_command;
> -	char data_buffer[1];
> +	char data_buffer[];
>  } TW_Ioctl_Buf_Apache;
>  
>  /* GetParam descriptor */
> @@ -344,7 +344,7 @@ typedef struct {
>  	unsigned short	parameter_id;
>  	unsigned short	parameter_size_bytes;
>  	unsigned short  actual_parameter_size_bytes;
> -	unsigned char	data[1];
> +	unsigned char	data[];
>  } TW_Param_Apache;
>  
>  /* Compatibility information structure */
> -- 
> 2.34.1
>
  
Martin K. Petersen Jan. 12, 2023, 5:10 a.m. UTC | #2
Kees,

> One-element arrays (and multi-element arrays being treated as
> dynamically sized) are deprecated[1] and are being replaced with
> flexible array members in support of the ongoing efforts to tighten
> the FORTIFY_SOURCE routines on memcpy(), correctly instrument array
> indexing with UBSAN_BOUNDS, and to globally enable
> -fstrict-flex-arrays=3.
>
> Replace one-element arrays with flexible-array member in
> TW_Ioctl_Buf_Apache and TW_Param_Apache, adjusting the explicit sizing
> calculations at the same time.
>
> This results in no differences in binary output.

Applied to 6.3/scsi-staging, thanks!
  
Martin K. Petersen Jan. 14, 2023, 3:06 a.m. UTC | #3
On Wed, 04 Jan 2023 16:48:01 -0800, Kees Cook wrote:

> One-element arrays (and multi-element arrays being treated as
> dynamically sized) are deprecated[1] and are being replaced with
> flexible array members in support of the ongoing efforts to tighten the
> FORTIFY_SOURCE routines on memcpy(), correctly instrument array indexing
> with UBSAN_BOUNDS, and to globally enable -fstrict-flex-arrays=3.
> 
> Replace one-element arrays with flexible-array member in TW_Ioctl_Buf_Apache
> and TW_Param_Apache, adjusting the explicit sizing calculations at the
> same time.
> 
> [...]

Applied to 6.3/scsi-queue, thanks!

[1/1] scsi: 3w-sas: Replace 1-element arrays with flexible array members
      https://git.kernel.org/mkp/scsi/c/45b379f20bc0
  

Patch

diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
index 3ebe66151dcb..f41c93454f0c 100644
--- a/drivers/scsi/3w-sas.c
+++ b/drivers/scsi/3w-sas.c
@@ -690,7 +690,7 @@  static void twl_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
 		newcommand->request_id__lunl =
 			cpu_to_le16(TW_REQ_LUN_IN(TW_LUN_OUT(newcommand->request_id__lunl), request_id));
 		if (length) {
-			newcommand->sg_list[0].address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache) - 1);
+			newcommand->sg_list[0].address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
 			newcommand->sg_list[0].length = TW_CPU_TO_SGL(length);
 		}
 		newcommand->sgl_entries__lunh =
@@ -702,7 +702,7 @@  static void twl_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
 		if (TW_SGL_OUT(oldcommand->opcode__sgloffset)) {
 			/* Load the sg list */
 			sgl = (TW_SG_Entry_ISO *)((u32 *)oldcommand+oldcommand->size - (sizeof(TW_SG_Entry_ISO)/4) + pae + (sizeof(dma_addr_t) > 4 ? 1 : 0));
-			sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache) - 1);
+			sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
 			sgl->length = TW_CPU_TO_SGL(length);
 			oldcommand->size += pae;
 			oldcommand->size += sizeof(dma_addr_t) > 4 ? 1 : 0;
@@ -748,7 +748,7 @@  static long twl_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 	data_buffer_length_adjusted = (driver_command.buffer_length + 511) & ~511;
 
 	/* Now allocate ioctl buf memory */
-	cpu_addr = dma_alloc_coherent(&tw_dev->tw_pci_dev->dev, data_buffer_length_adjusted+sizeof(TW_Ioctl_Buf_Apache) - 1, &dma_handle, GFP_KERNEL);
+	cpu_addr = dma_alloc_coherent(&tw_dev->tw_pci_dev->dev, data_buffer_length_adjusted + sizeof(TW_Ioctl_Buf_Apache), &dma_handle, GFP_KERNEL);
 	if (!cpu_addr) {
 		retval = -ENOMEM;
 		goto out2;
@@ -757,7 +757,7 @@  static long twl_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 	tw_ioctl = (TW_Ioctl_Buf_Apache *)cpu_addr;
 
 	/* Now copy down the entire ioctl */
-	if (copy_from_user(tw_ioctl, argp, driver_command.buffer_length + sizeof(TW_Ioctl_Buf_Apache) - 1))
+	if (copy_from_user(tw_ioctl, argp, driver_command.buffer_length + sizeof(TW_Ioctl_Buf_Apache)))
 		goto out3;
 
 	/* See which ioctl we are doing */
@@ -815,11 +815,11 @@  static long twl_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 	}
 
 	/* Now copy the entire response to userspace */
-	if (copy_to_user(argp, tw_ioctl, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length - 1) == 0)
+	if (copy_to_user(argp, tw_ioctl, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length) == 0)
 		retval = 0;
 out3:
 	/* Now free ioctl buf memory */
-	dma_free_coherent(&tw_dev->tw_pci_dev->dev, data_buffer_length_adjusted+sizeof(TW_Ioctl_Buf_Apache) - 1, cpu_addr, dma_handle);
+	dma_free_coherent(&tw_dev->tw_pci_dev->dev, data_buffer_length_adjusted + sizeof(TW_Ioctl_Buf_Apache), cpu_addr, dma_handle);
 out2:
 	mutex_unlock(&tw_dev->ioctl_lock);
 out:
diff --git a/drivers/scsi/3w-sas.h b/drivers/scsi/3w-sas.h
index b0508039a280..096dec29e2ac 100644
--- a/drivers/scsi/3w-sas.h
+++ b/drivers/scsi/3w-sas.h
@@ -335,7 +335,7 @@  typedef struct TAG_TW_Ioctl_Apache {
 	TW_Ioctl_Driver_Command driver_command;
 	char padding[488];
 	TW_Command_Full firmware_command;
-	char data_buffer[1];
+	char data_buffer[];
 } TW_Ioctl_Buf_Apache;
 
 /* GetParam descriptor */
@@ -344,7 +344,7 @@  typedef struct {
 	unsigned short	parameter_id;
 	unsigned short	parameter_size_bytes;
 	unsigned short  actual_parameter_size_bytes;
-	unsigned char	data[1];
+	unsigned char	data[];
 } TW_Param_Apache;
 
 /* Compatibility information structure */