* [PATCH v2 1/2] USB: EHCI: Add register array bounds to HCS ports
2021-08-18 17:30 [PATCH v2 0/2] USB: EHCI: Add register array bounds to HCS ports Kees Cook
@ 2021-08-18 17:30 ` Kees Cook
2021-08-18 17:30 ` [PATCH v2 2/2] USB: EHCI: Add alias for Broadcom INSNREG Kees Cook
2021-08-18 17:49 ` [PATCH v2 0/2] USB: EHCI: Add register array bounds to HCS ports Alan Stern
2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2021-08-18 17:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Arnd Bergmann, Al Cooper, Alan Stern, linux-usb,
Andy Shevchenko, Florian Fainelli, linux-kernel,
bcm-kernel-feedback-list, linux-hardening
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 <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Al Cooper <alcooperx@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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..dcbe2b068569 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 15 /* 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]; /* up to N_PORTS */
/* 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v2 2/2] USB: EHCI: Add alias for Broadcom INSNREG
2021-08-18 17:30 [PATCH v2 0/2] USB: EHCI: Add register array bounds to HCS ports Kees Cook
2021-08-18 17:30 ` [PATCH v2 1/2] " Kees Cook
@ 2021-08-18 17:30 ` Kees Cook
2021-08-18 17:49 ` [PATCH v2 0/2] USB: EHCI: Add register array bounds to HCS ports Alan Stern
2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2021-08-18 17:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Al Cooper, Alan Stern, linux-usb,
bcm-kernel-feedback-list, Arnd Bergmann, Andy Shevchenko,
Florian Fainelli, linux-kernel, linux-hardening
Refactor struct ehci_regs to avoid accessing beyond the end of
port_status. This change results in no difference in the final
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 was
confusing. If "USB_EHCI_INSNREG00" is at port_status[0x0f], its offset
would be 0x80 (not 0x90). The comments have been adjusted to fix this
apparent typo.
Cc: Al Cooper <alcooperx@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: bcm-kernel-feedback-list@broadcom.com
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 9df231511bd6 ("usb: ehci: Add new EHCI driver for Broadcom STB SoC's")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/usb/host/ehci-brcm.c | 11 ++++-------
include/linux/usb/ehci_def.h | 13 ++++++++++---
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
index 3e0ebe8cc649..d3626bfa966b 100644
--- a/drivers/usb/host/ehci-brcm.c
+++ b/drivers/usb/host/ehci-brcm.c
@@ -108,10 +108,9 @@ static int ehci_brcm_reset(struct usb_hcd *hcd)
/*
* SWLINUX-1705: Avoid OUT packet underflows during high memory
* 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[1]);
+ ehci_writel(ehci, 0x00000001, &ehci->regs->brcm_insnreg[3]);
return ehci_setup(hcd);
}
@@ -223,11 +222,9 @@ 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
*/
- 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[1]);
+ ehci_writel(ehci, 0x00000001, &ehci->regs->brcm_insnreg[3]);
ehci_resume(hcd, false);
diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
index dcbe2b068569..c892c5bc6638 100644
--- a/include/linux/usb/ehci_def.h
+++ b/include/linux/usb/ehci_def.h
@@ -176,16 +176,23 @@ struct ehci_regs {
#define USBMODE_CM_HC (3<<0) /* host controller mode */
#define USBMODE_CM_IDLE (0<<0) /* idle state */
};
- 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[HCS_N_PORTS_MAX];
+ union {
+ struct {
+ u32 reserved4;
+ /* 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 */
+ };
+
+ /* Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x80 */
+ u32 brcm_insnreg[4];
+ };
u32 reserved5[2];
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2 0/2] USB: EHCI: Add register array bounds to HCS ports
2021-08-18 17:30 [PATCH v2 0/2] USB: EHCI: Add register array bounds to HCS ports Kees Cook
2021-08-18 17:30 ` [PATCH v2 1/2] " Kees Cook
2021-08-18 17:30 ` [PATCH v2 2/2] USB: EHCI: Add alias for Broadcom INSNREG Kees Cook
@ 2021-08-18 17:49 ` Alan Stern
2 siblings, 0 replies; 4+ messages in thread
From: Alan Stern @ 2021-08-18 17:49 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, Arnd Bergmann, Al Cooper, Andy Shevchenko,
Florian Fainelli, linux-kernel, linux-usb,
bcm-kernel-feedback-list, linux-hardening
On Wed, Aug 18, 2021 at 10:30:16AM -0700, Kees Cook wrote:
> 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].
>
> No binary differences.
>
> v2:
> - use 0x80 as base for brcm registers (stern)
> - switch HCS_N_PORTS_MAX to decimal (stern)
> - update various comments (stern)
> v1: https://lore.kernel.org/lkml/20210818043035.1308062-1-keescook@chromium.org
>
> Thanks!
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/20200527134320.869042-1-arnd@arndb.de/#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 | 33 +++++++++++++++++++++------------
> 2 files changed, 25 insertions(+), 19 deletions(-)
For both patches:
Acked-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply [flat|nested] 4+ messages in thread