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 <[email protected]>
---
drivers/scsi/lpfc/lpfc_hw.h | 27 +++++++++++++++++----------
drivers/scsi/lpfc/lpfc_sli.c | 32 +++++++++++++++-----------------
2 files changed, 32 insertions(+), 27 deletions(-)
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);
--
2.30.2
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 <[email protected]>
> ---
looks fine.
Reviewed-by: James Smart <[email protected]>
-- james
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 Oracle Linux Engineering
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
--
Martin K. Petersen Oracle Linux Engineering