diff mbox series

scsi: lpfc: Use struct_group to isolate cast to larger object

Message ID 20211203223351.107323-1-keescook@chromium.org (mailing list archive)
State Mainlined
Commit c167dd0b2a7afcf2c25e44b9b6168cba3a51d27d
Headers show
Series scsi: lpfc: Use struct_group to isolate cast to larger object | expand

Commit Message

Kees Cook Dec. 3, 2021, 10:33 p.m. UTC
When building under -Warray-bounds, a warning is generated when casting
a u32 into MAILBOX_t (which is larger). This warning is conservative, but
it's not an unreasonable change to make to improve future robustness. Use
a tagged struct_group that can refer to either the specific fields or
the first u32 separately, silencing this warning:

drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_reset_barrier':
drivers/scsi/lpfc/lpfc_sli.c:4787:29: error: array subscript 'MAILBOX_t[0]' is partly outside array bounds of 'volatile uint32_t[1]' {aka 'volatile unsigned int[1]'} [-Werror=array-bounds]
 4787 |         ((MAILBOX_t *)&mbox)->mbxCommand = MBX_KILL_BOARD;
      |                             ^~
drivers/scsi/lpfc/lpfc_sli.c:4752:27: note: while referencing 'mbox'
 4752 |         volatile uint32_t mbox;
      |                           ^~~~

There is no change to the resulting executable instruction code.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/lpfc/lpfc_hw.h  | 27 +++++++++++++++++----------
 drivers/scsi/lpfc/lpfc_sli.c | 32 +++++++++++++++-----------------
 2 files changed, 32 insertions(+), 27 deletions(-)

Comments

James Smart Dec. 9, 2021, 2:50 p.m. UTC | #1
On 12/3/2021 2:33 PM, Kees Cook wrote:
> When building under -Warray-bounds, a warning is generated when casting
> a u32 into MAILBOX_t (which is larger). This warning is conservative, but
> it's not an unreasonable change to make to improve future robustness. Use
> a tagged struct_group that can refer to either the specific fields or
> the first u32 separately, silencing this warning:
> 
> drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_reset_barrier':
> drivers/scsi/lpfc/lpfc_sli.c:4787:29: error: array subscript 'MAILBOX_t[0]' is partly outside array bounds of 'volatile uint32_t[1]' {aka 'volatile unsigned int[1]'} [-Werror=array-bounds]
>   4787 |         ((MAILBOX_t *)&mbox)->mbxCommand = MBX_KILL_BOARD;
>        |                             ^~
> drivers/scsi/lpfc/lpfc_sli.c:4752:27: note: while referencing 'mbox'
>   4752 |         volatile uint32_t mbox;
>        |                           ^~~~
> 
> There is no change to the resulting executable instruction code.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

looks fine.

Reviewed-by: James Smart <jsmart2021@gmail.com>

-- james
Martin K. Petersen Dec. 14, 2021, 4:31 a.m. UTC | #2
Kees,

> When building under -Warray-bounds, a warning is generated when casting
> a u32 into MAILBOX_t (which is larger). This warning is conservative, but
> it's not an unreasonable change to make to improve future robustness. Use
> a tagged struct_group that can refer to either the specific fields or
> the first u32 separately, silencing this warning:

Applied to 5.17/scsi-staging, thanks!
Martin K. Petersen Dec. 17, 2021, 4:04 a.m. UTC | #3
On Fri, 3 Dec 2021 14:33:51 -0800, Kees Cook wrote:

> When building under -Warray-bounds, a warning is generated when casting
> a u32 into MAILBOX_t (which is larger). This warning is conservative, but
> it's not an unreasonable change to make to improve future robustness. Use
> a tagged struct_group that can refer to either the specific fields or
> the first u32 separately, silencing this warning:
> 
> drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_reset_barrier':
> drivers/scsi/lpfc/lpfc_sli.c:4787:29: error: array subscript 'MAILBOX_t[0]' is partly outside array bounds of 'volatile uint32_t[1]' {aka 'volatile unsigned int[1]'} [-Werror=array-bounds]
>  4787 |         ((MAILBOX_t *)&mbox)->mbxCommand = MBX_KILL_BOARD;
>       |                             ^~
> drivers/scsi/lpfc/lpfc_sli.c:4752:27: note: while referencing 'mbox'
>  4752 |         volatile uint32_t mbox;
>       |                           ^~~~
> 
> [...]

Applied to 5.17/scsi-queue, thanks!

[1/1] scsi: lpfc: Use struct_group to isolate cast to larger object
      https://git.kernel.org/mkp/scsi/c/c167dd0b2a7a
diff mbox series

Patch

diff --git a/drivers/scsi/lpfc/lpfc_hw.h b/drivers/scsi/lpfc/lpfc_hw.h
index 634f8fff7425..f0ce8404223a 100644
--- a/drivers/scsi/lpfc/lpfc_hw.h
+++ b/drivers/scsi/lpfc/lpfc_hw.h
@@ -3675,19 +3675,26 @@  union sli_var {
 };
 
 typedef struct {
+	struct_group_tagged(MAILBOX_word0, bits,
+		union {
+			struct {
 #ifdef __BIG_ENDIAN_BITFIELD
-	uint16_t mbxStatus;
-	uint8_t mbxCommand;
-	uint8_t mbxReserved:6;
-	uint8_t mbxHc:1;
-	uint8_t mbxOwner:1;	/* Low order bit first word */
+				uint16_t mbxStatus;
+				uint8_t mbxCommand;
+				uint8_t mbxReserved:6;
+				uint8_t mbxHc:1;
+				uint8_t mbxOwner:1;	/* Low order bit first word */
 #else	/*  __LITTLE_ENDIAN_BITFIELD */
-	uint8_t mbxOwner:1;	/* Low order bit first word */
-	uint8_t mbxHc:1;
-	uint8_t mbxReserved:6;
-	uint8_t mbxCommand;
-	uint16_t mbxStatus;
+				uint8_t mbxOwner:1;	/* Low order bit first word */
+				uint8_t mbxHc:1;
+				uint8_t mbxReserved:6;
+				uint8_t mbxCommand;
+				uint16_t mbxStatus;
 #endif
+			};
+			u32 word0;
+		};
+	);
 
 	MAILVARIANTS un;
 	union sli_var us;
diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 77dfe293bf23..8b3698032570 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -4749,7 +4749,7 @@  void lpfc_reset_barrier(struct lpfc_hba *phba)
 {
 	uint32_t __iomem *resp_buf;
 	uint32_t __iomem *mbox_buf;
-	volatile uint32_t mbox;
+	volatile struct MAILBOX_word0 mbox;
 	uint32_t hc_copy, ha_copy, resp_data;
 	int  i;
 	uint8_t hdrtype;
@@ -4783,13 +4783,13 @@  void lpfc_reset_barrier(struct lpfc_hba *phba)
 		phba->pport->stopped = 1;
 	}
 
-	mbox = 0;
-	((MAILBOX_t *)&mbox)->mbxCommand = MBX_KILL_BOARD;
-	((MAILBOX_t *)&mbox)->mbxOwner = OWN_CHIP;
+	mbox.word0 = 0;
+	mbox.mbxCommand = MBX_KILL_BOARD;
+	mbox.mbxOwner = OWN_CHIP;
 
 	writel(BARRIER_TEST_PATTERN, (resp_buf + 1));
 	mbox_buf = phba->MBslimaddr;
-	writel(mbox, mbox_buf);
+	writel(mbox.word0, mbox_buf);
 
 	for (i = 0; i < 50; i++) {
 		if (lpfc_readl((resp_buf + 1), &resp_data))
@@ -4810,12 +4810,12 @@  void lpfc_reset_barrier(struct lpfc_hba *phba)
 			goto clear_errat;
 	}
 
-	((MAILBOX_t *)&mbox)->mbxOwner = OWN_HOST;
+	mbox.mbxOwner = OWN_HOST;
 	resp_data = 0;
 	for (i = 0; i < 500; i++) {
 		if (lpfc_readl(resp_buf, &resp_data))
 			return;
-		if (resp_data != mbox)
+		if (resp_data != mbox.word0)
 			mdelay(1);
 		else
 			break;
@@ -5091,9 +5091,8 @@  lpfc_sli4_brdreset(struct lpfc_hba *phba)
 static int
 lpfc_sli_brdrestart_s3(struct lpfc_hba *phba)
 {
-	MAILBOX_t *mb;
+	volatile struct MAILBOX_word0 mb;
 	struct lpfc_sli *psli;
-	volatile uint32_t word0;
 	void __iomem *to_slim;
 	uint32_t hba_aer_enabled;
 
@@ -5110,24 +5109,23 @@  lpfc_sli_brdrestart_s3(struct lpfc_hba *phba)
 			(phba->pport) ? phba->pport->port_state : 0,
 			psli->sli_flag);
 
-	word0 = 0;
-	mb = (MAILBOX_t *) &word0;
-	mb->mbxCommand = MBX_RESTART;
-	mb->mbxHc = 1;
+	mb.word0 = 0;
+	mb.mbxCommand = MBX_RESTART;
+	mb.mbxHc = 1;
 
 	lpfc_reset_barrier(phba);
 
 	to_slim = phba->MBslimaddr;
-	writel(*(uint32_t *) mb, to_slim);
+	writel(mb.word0, to_slim);
 	readl(to_slim); /* flush */
 
 	/* Only skip post after fc_ffinit is completed */
 	if (phba->pport && phba->pport->port_state)
-		word0 = 1;	/* This is really setting up word1 */
+		mb.word0 = 1;	/* This is really setting up word1 */
 	else
-		word0 = 0;	/* This is really setting up word1 */
+		mb.word0 = 0;	/* This is really setting up word1 */
 	to_slim = phba->MBslimaddr + sizeof (uint32_t);
-	writel(*(uint32_t *) mb, to_slim);
+	writel(mb.word0, to_slim);
 	readl(to_slim); /* flush */
 
 	lpfc_sli_brdreset(phba);