Hi,
This is cleaning up some of the remaining things to be able to apply
-Warray-bounds and -Wzero-length-bounds globally. Only after doing my
own version of the port_status patch did I find Arnd's earlier
patches, including for the weird Broadcom stuff[1].
Anyway, here's what I got. :) No binary differences.
-Kees
[1] https://lore.kernel.org/lkml/[email protected]/#t
Kees Cook (2):
USB: EHCI: Add register array bounds to HCS ports
USB: EHCI: Add alias for Broadcom INSNREG
drivers/usb/host/ehci-brcm.c | 11 +++++------
include/linux/usb/ehci_def.h | 38 ++++++++++++++++++++++++------------
2 files changed, 31 insertions(+), 18 deletions(-)
--
2.30.2
The original EHCI register struct used a trailing 0-element array for
addressing the N_PORTS-many available registers. However, after
commit a46af4ebf9ff ("USB: EHCI: define extension registers like normal ones")
the 0-element array started to overlap the USBMODE extension register.
To avoid future compile-time warnings about accessing indexes within a
0-element array, rearrange the struct to actually describe the expected
layout (max 15 registers) with a union. All offsets remain the same, and
bounds checking becomes possible on accesses to port_status and hostpc.
There are no binary differences, and struct offsets continue to match.
"pahole --hex -C ehci_regs" before:
struct ehci_regs {
u32 command; /* 0 0x4 */
u32 status; /* 0x4 0x4 */
u32 intr_enable; /* 0x8 0x4 */
u32 frame_index; /* 0xc 0x4 */
u32 segment; /* 0x10 0x4 */
u32 frame_list; /* 0x14 0x4 */
u32 async_next; /* 0x18 0x4 */
u32 reserved1[2]; /* 0x1c 0x8 */
u32 txfill_tuning; /* 0x24 0x4 */
u32 reserved2[6]; /* 0x28 0x18 */
/* --- cacheline 1 boundary (64 bytes) --- */
u32 configured_flag; /* 0x40 0x4 */
u32 port_status[0]; /* 0x44 0 */
u32 reserved3[9]; /* 0x44 0x24 */
u32 usbmode; /* 0x68 0x4 */
u32 reserved4[6]; /* 0x6c 0x18 */
/* --- cacheline 2 boundary (128 bytes) was 4 bytes ago --- */
u32 hostpc[0]; /* 0x84 0 */
u32 reserved5[17]; /* 0x84 0x44 */
/* --- cacheline 3 boundary (192 bytes) was 8 bytes ago --- */
u32 usbmode_ex; /* 0xc8 0x4 */
/* size: 204, cachelines: 4, members: 18 */
/* last cacheline: 12 bytes */
};
after:
struct ehci_regs {
u32 command; /* 0 0x4 */
u32 status; /* 0x4 0x4 */
u32 intr_enable; /* 0x8 0x4 */
u32 frame_index; /* 0xc 0x4 */
u32 segment; /* 0x10 0x4 */
u32 frame_list; /* 0x14 0x4 */
u32 async_next; /* 0x18 0x4 */
u32 reserved1[2]; /* 0x1c 0x8 */
u32 txfill_tuning; /* 0x24 0x4 */
u32 reserved2[6]; /* 0x28 0x18 */
/* --- cacheline 1 boundary (64 bytes) --- */
u32 configured_flag; /* 0x40 0x4 */
union {
u32 port_status[15]; /* 0x44 0x3c */
struct {
u32 reserved3[9]; /* 0x44 0x24 */
u32 usbmode; /* 0x68 0x4 */
}; /* 0x44 0x28 */
}; /* 0x44 0x3c */
/* --- cacheline 2 boundary (128 bytes) --- */
u32 reserved4; /* 0x80 0x4 */
u32 hostpc[15]; /* 0x84 0x3c */
/* --- cacheline 3 boundary (192 bytes) --- */
u32 reserved5[2]; /* 0xc0 0x8 */
u32 usbmode_ex; /* 0xc8 0x4 */
/* size: 204, cachelines: 4, members: 16 */
/* last cacheline: 12 bytes */
};
With this fixed, adding -Wzero-length-bounds to the build no longer
produces several warnings like this:
In file included from drivers/usb/host/ehci-hcd.c:306:
drivers/usb/host/ehci-hub.c: In function 'ehci_port_handed_over':
drivers/usb/host/ehci-hub.c:1194:8: warning: array subscript '<unknown>' is outside the bounds of an interior zero-length array 'u32[0]' {aka 'unsigned int[]'} [-Wzero-length-bounds]
1194 | reg = &ehci->regs->port_status[portnum - 1];
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/usb/host/ehci.h:274,
from drivers/usb/host/ehci-hcd.c:97:
./include/linux/usb/ehci_def.h:130:7: note: while referencing 'port_status'
130 | u32 port_status[0]; /* up to N_PORTS */
| ^~~~~~~~~~~
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Al Cooper <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: [email protected]
Fixes: a46af4ebf9ff ("USB: EHCI: define extension registers like normal ones")
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/usb/ehci_def.h | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
index 78e006355557..5398f571113b 100644
--- a/include/linux/usb/ehci_def.h
+++ b/include/linux/usb/ehci_def.h
@@ -45,6 +45,7 @@ struct ehci_caps {
#define HCS_PORTROUTED(p) ((p)&(1 << 7)) /* true: port routing */
#define HCS_PPC(p) ((p)&(1 << 4)) /* true: port power control */
#define HCS_N_PORTS(p) (((p)>>0)&0xf) /* bits 3:0, ports on HC */
+#define HCS_N_PORTS_MAX 0xf /* N_PORTS valid 0x1-0xF */
u32 hcc_params; /* HCCPARAMS - offset 0x8 */
/* EHCI 1.1 addendum */
@@ -126,8 +127,9 @@ struct ehci_regs {
u32 configured_flag;
#define FLAG_CF (1<<0) /* true: we'll support "high speed" */
- /* PORTSC: offset 0x44 */
- u32 port_status[0]; /* up to N_PORTS */
+ union {
+ /* PORTSC: offset 0x44 */
+ u32 port_status[HCS_N_PORTS_MAX];
/* EHCI 1.1 addendum */
#define PORTSC_SUSPEND_STS_ACK 0
#define PORTSC_SUSPEND_STS_NYET 1
@@ -164,28 +166,28 @@ struct ehci_regs {
#define PORT_CSC (1<<1) /* connect status change */
#define PORT_CONNECT (1<<0) /* device connected */
#define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
-
- u32 reserved3[9];
-
- /* USBMODE: offset 0x68 */
- u32 usbmode; /* USB Device mode */
+ struct {
+ u32 reserved3[9];
+ /* USBMODE: offset 0x68 */
+ u32 usbmode; /* USB Device mode */
+ };
#define USBMODE_SDIS (1<<3) /* Stream disable */
#define USBMODE_BE (1<<2) /* BE/LE endianness select */
#define USBMODE_CM_HC (3<<0) /* host controller mode */
#define USBMODE_CM_IDLE (0<<0) /* idle state */
-
- u32 reserved4[6];
+ };
+ u32 reserved4;
/* Moorestown has some non-standard registers, partially due to the fact that
* its EHCI controller has both TT and LPM support. HOSTPCx are extensions to
* PORTSCx
*/
/* HOSTPC: offset 0x84 */
- u32 hostpc[0]; /* HOSTPC extension */
+ u32 hostpc[HCS_N_PORTS_MAX];
#define HOSTPC_PHCD (1<<22) /* Phy clock disable */
#define HOSTPC_PSPD (3<<25) /* Port speed detection */
- u32 reserved5[17];
+ u32 reserved5[2];
/* USBMODE_EX: offset 0xc8 */
u32 usbmode_ex; /* USB Device mode extension */
--
2.30.2
Refactor struct ehci_regs to avoid accessing beyond the end of
port_status. This change results in no difference in the resulting
object code.
Avoids several warnings when building with -Warray-bounds:
drivers/usb/host/ehci-brcm.c: In function 'ehci_brcm_reset':
drivers/usb/host/ehci-brcm.c:113:32: warning: array subscript 16 is above array bounds of 'u32[15]' {aka 'unsigned int[15]'} [-Warray-bounds]
113 | ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/usb/host/ehci.h:274,
from drivers/usb/host/ehci-brcm.c:15:
./include/linux/usb/ehci_def.h:132:7: note: while referencing 'port_status'
132 | u32 port_status[HCS_N_PORTS_MAX];
| ^~~~~~~~~~~
Note that the documentation around this proprietary register is
confusing. If "USB_EHCI_INSNREG00" is at port_status[0x0f], its offset
would be 0x80 (not 0x90). The code uses port_status[0x10], so is that
not using "USB_EHCI_INSNREG00"?
Perhaps port_status[0x10] is USB_EHCI_INSNREG01 and port_status[0x12]
is USB_EHCI_INSNREG03? If so, the union could be adjusted to better
represent the layout.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Al Cooper <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: [email protected]
Cc: [email protected]
Suggested-by: Arnd Bergmann <[email protected]>
Fixes: 9df231511bd6 ("usb: ehci: Add new EHCI driver for Broadcom STB SoC's")
Signed-off-by: Kees Cook <[email protected]>
---
drivers/usb/host/ehci-brcm.c | 11 +++++------
include/linux/usb/ehci_def.h | 16 ++++++++++++++--
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
index 3e0ebe8cc649..5d232d3701f9 100644
--- a/drivers/usb/host/ehci-brcm.c
+++ b/drivers/usb/host/ehci-brcm.c
@@ -110,8 +110,8 @@ static int ehci_brcm_reset(struct usb_hcd *hcd)
* bus usage
* port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
*/
- ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
- ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
+ ehci_writel(ehci, 0x00800040, &ehci->regs->brcm_insnreg[0]);
+ ehci_writel(ehci, 0x00000001, &ehci->regs->brcm_insnreg[2]);
return ehci_setup(hcd);
}
@@ -223,11 +223,10 @@ static int __maybe_unused ehci_brcm_resume(struct device *dev)
/*
* SWLINUX-1705: Avoid OUT packet underflows during high memory
* bus usage
- * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00
- * @ 0x90
+ * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
*/
- ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
- ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
+ ehci_writel(ehci, 0x00800040, &ehci->regs->brcm_insnreg[0]);
+ ehci_writel(ehci, 0x00000001, &ehci->regs->brcm_insnreg[2]);
ehci_resume(hcd, false);
diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
index 5398f571113b..86f0909cab99 100644
--- a/include/linux/usb/ehci_def.h
+++ b/include/linux/usb/ehci_def.h
@@ -182,11 +182,23 @@ struct ehci_regs {
* its EHCI controller has both TT and LPM support. HOSTPCx are extensions to
* PORTSCx
*/
- /* HOSTPC: offset 0x84 */
- u32 hostpc[HCS_N_PORTS_MAX];
+ union {
+ /* HOSTPC: offset 0x84 */
+ u32 hostpc[HCS_N_PORTS_MAX];
#define HOSTPC_PHCD (1<<22) /* Phy clock disable */
#define HOSTPC_PSPD (3<<25) /* Port speed detection */
+ /*
+ * This was originally documented as:
+ * "port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90"
+ * but this doesn't make sense: the code was using
+ * port_status[0x10]. port_status[0x0f] would be reserved4.
+ * Also, none of these are near 0x90. port_status[0x10] is
+ * offset 0x84, and port_status[0x0f] would be 0x80.
+ */
+ u32 brcm_insnreg[3];
+ };
+
u32 reserved5[2];
/* USBMODE_EX: offset 0xc8 */
--
2.30.2
On Wed, Aug 18, 2021 at 7:30 AM Kees Cook <[email protected]> wrote:
>
> The original EHCI register struct used a trailing 0-element array for
> addressing the N_PORTS-many available registers. However, after
> commit a46af4ebf9ff ("USB: EHCI: define extension registers like normal ones")
> the 0-element array started to overlap the USBMODE extension register.
>
> To avoid future compile-time warnings about accessing indexes within a
> 0-element array, rearrange the struct to actually describe the expected
> layout (max 15 registers) with a union. All offsets remain the same, and
> bounds checking becomes possible on accesses to port_status and hostpc.
...
> /* HOSTPC: offset 0x84 */
> - u32 hostpc[0]; /* HOSTPC extension */
> + u32 hostpc[HCS_N_PORTS_MAX];
> #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> #define HOSTPC_PSPD (3<<25) /* Port speed detection */
>
> - u32 reserved5[17];
> + u32 reserved5[2];
Shouldn't it be rather [17 - PORT_MAX]? for accuracy?
Or also a union approach?
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 17, 2021 at 09:30:33PM -0700, Kees Cook wrote:
> The original EHCI register struct used a trailing 0-element array for
> addressing the N_PORTS-many available registers. However, after
> commit a46af4ebf9ff ("USB: EHCI: define extension registers like normal ones")
> the 0-element array started to overlap the USBMODE extension register.
>
> To avoid future compile-time warnings about accessing indexes within a
> 0-element array, rearrange the struct to actually describe the expected
> layout (max 15 registers) with a union. All offsets remain the same, and
> bounds checking becomes possible on accesses to port_status and hostpc.
>
> There are no binary differences, and struct offsets continue to match.
Two comments...
> ---
> include/linux/usb/ehci_def.h | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
> index 78e006355557..5398f571113b 100644
> --- a/include/linux/usb/ehci_def.h
> +++ b/include/linux/usb/ehci_def.h
> @@ -45,6 +45,7 @@ struct ehci_caps {
> #define HCS_PORTROUTED(p) ((p)&(1 << 7)) /* true: port routing */
> #define HCS_PPC(p) ((p)&(1 << 4)) /* true: port power control */
> #define HCS_N_PORTS(p) (((p)>>0)&0xf) /* bits 3:0, ports on HC */
> +#define HCS_N_PORTS_MAX 0xf /* N_PORTS valid 0x1-0xF */
I would prefer to see this value in decimal. It seems very odd to say
something like "The maximum number of ports is 0xf".
>
> u32 hcc_params; /* HCCPARAMS - offset 0x8 */
> /* EHCI 1.1 addendum */
> @@ -126,8 +127,9 @@ struct ehci_regs {
> u32 configured_flag;
> #define FLAG_CF (1<<0) /* true: we'll support "high speed" */
>
> - /* PORTSC: offset 0x44 */
> - u32 port_status[0]; /* up to N_PORTS */
> + union {
> + /* PORTSC: offset 0x44 */
> + u32 port_status[HCS_N_PORTS_MAX];
Please don't lose the second comment.
> /* EHCI 1.1 addendum */
> #define PORTSC_SUSPEND_STS_ACK 0
> #define PORTSC_SUSPEND_STS_NYET 1
> @@ -164,28 +166,28 @@ struct ehci_regs {
> #define PORT_CSC (1<<1) /* connect status change */
> #define PORT_CONNECT (1<<0) /* device connected */
> #define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
> -
> - u32 reserved3[9];
> -
> - /* USBMODE: offset 0x68 */
> - u32 usbmode; /* USB Device mode */
> + struct {
> + u32 reserved3[9];
> + /* USBMODE: offset 0x68 */
> + u32 usbmode; /* USB Device mode */
> + };
> #define USBMODE_SDIS (1<<3) /* Stream disable */
> #define USBMODE_BE (1<<2) /* BE/LE endianness select */
> #define USBMODE_CM_HC (3<<0) /* host controller mode */
> #define USBMODE_CM_IDLE (0<<0) /* idle state */
> -
> - u32 reserved4[6];
> + };
> + u32 reserved4;
>
> /* Moorestown has some non-standard registers, partially due to the fact that
> * its EHCI controller has both TT and LPM support. HOSTPCx are extensions to
> * PORTSCx
> */
> /* HOSTPC: offset 0x84 */
> - u32 hostpc[0]; /* HOSTPC extension */
> + u32 hostpc[HCS_N_PORTS_MAX];
> #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> #define HOSTPC_PSPD (3<<25) /* Port speed detection */
>
> - u32 reserved5[17];
> + u32 reserved5[2];
>
> /* USBMODE_EX: offset 0xc8 */
> u32 usbmode_ex; /* USB Device mode extension */
Otherwise okay.
Alan Stern
On Tue, Aug 17, 2021 at 09:30:34PM -0700, Kees Cook wrote:
> Refactor struct ehci_regs to avoid accessing beyond the end of
> port_status. This change results in no difference in the resulting
> object code.
>
> Avoids several warnings when building with -Warray-bounds:
>
> drivers/usb/host/ehci-brcm.c: In function 'ehci_brcm_reset':
> drivers/usb/host/ehci-brcm.c:113:32: warning: array subscript 16 is above array bounds of 'u32[15]' {aka 'unsigned int[15]'} [-Warray-bounds]
> 113 | ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from drivers/usb/host/ehci.h:274,
> from drivers/usb/host/ehci-brcm.c:15:
> ./include/linux/usb/ehci_def.h:132:7: note: while referencing 'port_status'
> 132 | u32 port_status[HCS_N_PORTS_MAX];
> | ^~~~~~~~~~~
>
> Note that the documentation around this proprietary register is
> confusing. If "USB_EHCI_INSNREG00" is at port_status[0x0f], its offset
> would be 0x80 (not 0x90). The code uses port_status[0x10], so is that
> not using "USB_EHCI_INSNREG00"?
I suspect the 0x90 value in the comment is a typo for 0x80.
> Perhaps port_status[0x10] is USB_EHCI_INSNREG01 and port_status[0x12]
> is USB_EHCI_INSNREG03? If so, the union could be adjusted to better
> represent the layout.
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Al Cooper <[email protected]>
> Cc: Alan Stern <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Suggested-by: Arnd Bergmann <[email protected]>
> Fixes: 9df231511bd6 ("usb: ehci: Add new EHCI driver for Broadcom STB SoC's")
> Signed-off-by: Kees Cook <[email protected]>
> ---
> drivers/usb/host/ehci-brcm.c | 11 +++++------
> include/linux/usb/ehci_def.h | 16 ++++++++++++++--
> 2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
> index 3e0ebe8cc649..5d232d3701f9 100644
> --- a/drivers/usb/host/ehci-brcm.c
> +++ b/drivers/usb/host/ehci-brcm.c
> @@ -110,8 +110,8 @@ static int ehci_brcm_reset(struct usb_hcd *hcd)
> * bus usage
> * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
This last comment line is no longer necessary, thanks to the revised
port definitions. And since it is actively misleading, with the 0x90
instead of 0x80, I think it should be removed entirely.
> */
> - ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> - ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
> + ehci_writel(ehci, 0x00800040, &ehci->regs->brcm_insnreg[0]);
> + ehci_writel(ehci, 0x00000001, &ehci->regs->brcm_insnreg[2]);
>
> return ehci_setup(hcd);
> }
> @@ -223,11 +223,10 @@ static int __maybe_unused ehci_brcm_resume(struct device *dev)
> /*
> * SWLINUX-1705: Avoid OUT packet underflows during high memory
> * bus usage
> - * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00
> - * @ 0x90
> + * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
Same here.
> */
> - ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> - ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
> + ehci_writel(ehci, 0x00800040, &ehci->regs->brcm_insnreg[0]);
> + ehci_writel(ehci, 0x00000001, &ehci->regs->brcm_insnreg[2]);
>
> ehci_resume(hcd, false);
>
> diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
> index 5398f571113b..86f0909cab99 100644
> --- a/include/linux/usb/ehci_def.h
> +++ b/include/linux/usb/ehci_def.h
> @@ -182,11 +182,23 @@ struct ehci_regs {
> * its EHCI controller has both TT and LPM support. HOSTPCx are extensions to
> * PORTSCx
> */
> - /* HOSTPC: offset 0x84 */
> - u32 hostpc[HCS_N_PORTS_MAX];
> + union {
> + /* HOSTPC: offset 0x84 */
> + u32 hostpc[HCS_N_PORTS_MAX];
> #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> #define HOSTPC_PSPD (3<<25) /* Port speed detection */
>
> + /*
> + * This was originally documented as:
> + * "port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90"
> + * but this doesn't make sense: the code was using
> + * port_status[0x10]. port_status[0x0f] would be reserved4.
> + * Also, none of these are near 0x90. port_status[0x10] is
> + * offset 0x84, and port_status[0x0f] would be 0x80.
> + */
This comment is entirely inappropriate. It's the sort of thing that
belongs in the git history, not in the code.
> + u32 brcm_insnreg[3];
Given the notation in the original comments, perhaps it would be better
to define this as:
struct { /* Broadcom proprietary registers */
u32 brcm_insnreg01; /* offset 0x84 */
u32 brcm_insnreg02;
u32 brcm_insnreg03;
};
I don't know. It would be nice to hear from somebody at Broadcom.
Alan Stern
> + };
> +
> u32 reserved5[2];
>
> /* USBMODE_EX: offset 0xc8 */
> --
> 2.30.2
>
On Wed, Aug 18, 2021 at 12:48:17PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 18, 2021 at 7:30 AM Kees Cook <[email protected]> wrote:
> >
> > The original EHCI register struct used a trailing 0-element array for
> > addressing the N_PORTS-many available registers. However, after
> > commit a46af4ebf9ff ("USB: EHCI: define extension registers like normal ones")
> > the 0-element array started to overlap the USBMODE extension register.
> >
> > To avoid future compile-time warnings about accessing indexes within a
> > 0-element array, rearrange the struct to actually describe the expected
> > layout (max 15 registers) with a union. All offsets remain the same, and
> > bounds checking becomes possible on accesses to port_status and hostpc.
>
> ...
>
> > /* HOSTPC: offset 0x84 */
> > - u32 hostpc[0]; /* HOSTPC extension */
> > + u32 hostpc[HCS_N_PORTS_MAX];
> > #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> > #define HOSTPC_PSPD (3<<25) /* Port speed detection */
> >
> > - u32 reserved5[17];
> > + u32 reserved5[2];
>
> Shouldn't it be rather [17 - PORT_MAX]? for accuracy?
> Or also a union approach?
It's okay to use [2] here. The only purpose is to ensure that the
following usbmode_ex field is allocated at offset 0xc8; there's no
special intrinsic meaning to that 17 value.
Alan Stern
On Wed, Aug 18, 2021 at 10:57:36AM -0400, Alan Stern wrote:
> On Tue, Aug 17, 2021 at 09:30:34PM -0700, Kees Cook wrote:
> > Refactor struct ehci_regs to avoid accessing beyond the end of
> > port_status. This change results in no difference in the resulting
> > object code.
> >
> > Avoids several warnings when building with -Warray-bounds:
> >
> > drivers/usb/host/ehci-brcm.c: In function 'ehci_brcm_reset':
> > drivers/usb/host/ehci-brcm.c:113:32: warning: array subscript 16 is above array bounds of 'u32[15]' {aka 'unsigned int[15]'} [-Warray-bounds]
> > 113 | ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > In file included from drivers/usb/host/ehci.h:274,
> > from drivers/usb/host/ehci-brcm.c:15:
> > ./include/linux/usb/ehci_def.h:132:7: note: while referencing 'port_status'
> > 132 | u32 port_status[HCS_N_PORTS_MAX];
> > | ^~~~~~~~~~~
> >
> > Note that the documentation around this proprietary register is
> > confusing. If "USB_EHCI_INSNREG00" is at port_status[0x0f], its offset
> > would be 0x80 (not 0x90). The code uses port_status[0x10], so is that
> > not using "USB_EHCI_INSNREG00"?
>
> I suspect the 0x90 value in the comment is a typo for 0x80.
That'd be my conclusion too. I've updated this for v2.
>
> > Perhaps port_status[0x10] is USB_EHCI_INSNREG01 and port_status[0x12]
> > is USB_EHCI_INSNREG03? If so, the union could be adjusted to better
> > represent the layout.
> >
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Al Cooper <[email protected]>
> > Cc: Alan Stern <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Suggested-by: Arnd Bergmann <[email protected]>
> > Fixes: 9df231511bd6 ("usb: ehci: Add new EHCI driver for Broadcom STB SoC's")
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > drivers/usb/host/ehci-brcm.c | 11 +++++------
> > include/linux/usb/ehci_def.h | 16 ++++++++++++++--
> > 2 files changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
> > index 3e0ebe8cc649..5d232d3701f9 100644
> > --- a/drivers/usb/host/ehci-brcm.c
> > +++ b/drivers/usb/host/ehci-brcm.c
> > @@ -110,8 +110,8 @@ static int ehci_brcm_reset(struct usb_hcd *hcd)
> > * bus usage
> > * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
>
> This last comment line is no longer necessary, thanks to the revised
> port definitions. And since it is actively misleading, with the 0x90
> instead of 0x80, I think it should be removed entirely.
Done.
>
> > */
> > - ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> > - ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
> > + ehci_writel(ehci, 0x00800040, &ehci->regs->brcm_insnreg[0]);
> > + ehci_writel(ehci, 0x00000001, &ehci->regs->brcm_insnreg[2]);
> >
> > return ehci_setup(hcd);
> > }
> > @@ -223,11 +223,10 @@ static int __maybe_unused ehci_brcm_resume(struct device *dev)
> > /*
> > * SWLINUX-1705: Avoid OUT packet underflows during high memory
> > * bus usage
> > - * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00
> > - * @ 0x90
> > + * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
>
> Same here.
>
> > */
> > - ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> > - ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
> > + ehci_writel(ehci, 0x00800040, &ehci->regs->brcm_insnreg[0]);
> > + ehci_writel(ehci, 0x00000001, &ehci->regs->brcm_insnreg[2]);
> >
> > ehci_resume(hcd, false);
> >
> > diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
> > index 5398f571113b..86f0909cab99 100644
> > --- a/include/linux/usb/ehci_def.h
> > +++ b/include/linux/usb/ehci_def.h
> > @@ -182,11 +182,23 @@ struct ehci_regs {
> > * its EHCI controller has both TT and LPM support. HOSTPCx are extensions to
> > * PORTSCx
> > */
> > - /* HOSTPC: offset 0x84 */
> > - u32 hostpc[HCS_N_PORTS_MAX];
> > + union {
> > + /* HOSTPC: offset 0x84 */
> > + u32 hostpc[HCS_N_PORTS_MAX];
> > #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> > #define HOSTPC_PSPD (3<<25) /* Port speed detection */
> >
> > + /*
> > + * This was originally documented as:
> > + * "port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90"
> > + * but this doesn't make sense: the code was using
> > + * port_status[0x10]. port_status[0x0f] would be reserved4.
> > + * Also, none of these are near 0x90. port_status[0x10] is
> > + * offset 0x84, and port_status[0x0f] would be 0x80.
> > + */
>
> This comment is entirely inappropriate. It's the sort of thing that
> belongs in the git history, not in the code.
I wanted it to be easily discoverable, but since we've got a preferred
result now, I'm dropping this and orienting against 0x80.
>
> > + u32 brcm_insnreg[3];
>
> Given the notation in the original comments, perhaps it would be better
> to define this as:
>
> struct { /* Broadcom proprietary registers */
> u32 brcm_insnreg01; /* offset 0x84 */
> u32 brcm_insnreg02;
> u32 brcm_insnreg03;
> };
Following the other register arrays, I'm going to keep an array for
this, but adjust the numbering to start at 0 @ 0x80 so the code will
poke offset 1 and 3.
> I don't know. It would be nice to hear from somebody at Broadcom.
Agreed. :)
Thanks for the review!
--
Kees Cook
On Wed, Aug 18, 2021 at 10:44:42AM -0400, Alan Stern wrote:
> On Tue, Aug 17, 2021 at 09:30:33PM -0700, Kees Cook wrote:
> > The original EHCI register struct used a trailing 0-element array for
> > addressing the N_PORTS-many available registers. However, after
> > commit a46af4ebf9ff ("USB: EHCI: define extension registers like normal ones")
> > the 0-element array started to overlap the USBMODE extension register.
> >
> > To avoid future compile-time warnings about accessing indexes within a
> > 0-element array, rearrange the struct to actually describe the expected
> > layout (max 15 registers) with a union. All offsets remain the same, and
> > bounds checking becomes possible on accesses to port_status and hostpc.
> >
> > There are no binary differences, and struct offsets continue to match.
>
> Two comments...
>
> > ---
> > include/linux/usb/ehci_def.h | 24 +++++++++++++-----------
> > 1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
> > index 78e006355557..5398f571113b 100644
> > --- a/include/linux/usb/ehci_def.h
> > +++ b/include/linux/usb/ehci_def.h
> > @@ -45,6 +45,7 @@ struct ehci_caps {
> > #define HCS_PORTROUTED(p) ((p)&(1 << 7)) /* true: port routing */
> > #define HCS_PPC(p) ((p)&(1 << 4)) /* true: port power control */
> > #define HCS_N_PORTS(p) (((p)>>0)&0xf) /* bits 3:0, ports on HC */
> > +#define HCS_N_PORTS_MAX 0xf /* N_PORTS valid 0x1-0xF */
>
> I would prefer to see this value in decimal. It seems very odd to say
> something like "The maximum number of ports is 0xf".
Okay, done.
>
> >
> > u32 hcc_params; /* HCCPARAMS - offset 0x8 */
> > /* EHCI 1.1 addendum */
> > @@ -126,8 +127,9 @@ struct ehci_regs {
> > u32 configured_flag;
> > #define FLAG_CF (1<<0) /* true: we'll support "high speed" */
> >
> > - /* PORTSC: offset 0x44 */
> > - u32 port_status[0]; /* up to N_PORTS */
> > + union {
> > + /* PORTSC: offset 0x44 */
> > + u32 port_status[HCS_N_PORTS_MAX];
>
> Please don't lose the second comment.
I've put it back. It seemed redundant in the face of HCS_N_PORTS_MAX
being there now.
>
> > /* EHCI 1.1 addendum */
> > #define PORTSC_SUSPEND_STS_ACK 0
> > #define PORTSC_SUSPEND_STS_NYET 1
> > @@ -164,28 +166,28 @@ struct ehci_regs {
> > #define PORT_CSC (1<<1) /* connect status change */
> > #define PORT_CONNECT (1<<0) /* device connected */
> > #define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_OCC)
> > -
> > - u32 reserved3[9];
> > -
> > - /* USBMODE: offset 0x68 */
> > - u32 usbmode; /* USB Device mode */
> > + struct {
> > + u32 reserved3[9];
> > + /* USBMODE: offset 0x68 */
> > + u32 usbmode; /* USB Device mode */
> > + };
> > #define USBMODE_SDIS (1<<3) /* Stream disable */
> > #define USBMODE_BE (1<<2) /* BE/LE endianness select */
> > #define USBMODE_CM_HC (3<<0) /* host controller mode */
> > #define USBMODE_CM_IDLE (0<<0) /* idle state */
> > -
> > - u32 reserved4[6];
> > + };
> > + u32 reserved4;
> >
> > /* Moorestown has some non-standard registers, partially due to the fact that
> > * its EHCI controller has both TT and LPM support. HOSTPCx are extensions to
> > * PORTSCx
> > */
> > /* HOSTPC: offset 0x84 */
> > - u32 hostpc[0]; /* HOSTPC extension */
> > + u32 hostpc[HCS_N_PORTS_MAX];
> > #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> > #define HOSTPC_PSPD (3<<25) /* Port speed detection */
> >
> > - u32 reserved5[17];
> > + u32 reserved5[2];
> >
> > /* USBMODE_EX: offset 0xc8 */
> > u32 usbmode_ex; /* USB Device mode extension */
>
> Otherwise okay.
Thanks!
--
Kees Cook