Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1404025 > unrolled thread
| Started by | Valdis Kletnieks <Valdis.Kletnieks@vt.edu> |
|---|---|
| First post | 2016-05-20 02:00 +0200 |
| Last post | 2016-05-20 16:40 +0200 |
| Articles | 5 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] - silence UBSAN complaint in ehci-hcd. Valdis Kletnieks <Valdis.Kletnieks@vt.edu> - 2016-05-20 02:00 +0200
Re: [PATCH] - silence UBSAN complaint in ehci-hcd. Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-05-20 03:00 +0200
Re: [PATCH] - silence UBSAN complaint in ehci-hcd. Valdis.Kletnieks@vt.edu - 2016-05-20 03:10 +0200
Re: [PATCH] - silence UBSAN complaint in ehci-hcd. Alan Stern <stern@rowland.harvard.edu> - 2016-05-20 16:40 +0200
Re: [PATCH] - silence UBSAN complaint in ehci-hcd. Alan Stern <stern@rowland.harvard.edu> - 2016-05-20 16:40 +0200
| From | Valdis Kletnieks <Valdis.Kletnieks@vt.edu> |
|---|---|
| Date | 2016-05-20 02:00 +0200 |
| Subject | [PATCH] - silence UBSAN complaint in ehci-hcd. |
| Message-ID | <rAG1b-6u9-11@gated-at.bofh.it> |
UBSAN throws a complaint:
[ 2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
[ 2.418582] index -1 is out of range for type 'u32 [1]'
though it's only on the hostpc[] part, not on the port_status[] on the
previous line which has the same exact index calculation. The root cause is
that the first declaration is port_status[0], which uses a GCC extension and
UBSAN is smart enough to realize the programmer is doing something
intentionally odd.
However, the problematic declaration is hostpc[1], which doesn't have
the "I know what I'm doing" semantics of [0]. Change the declaration to match.
Signed-Off-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
--- a/include/linux/usb/ehci_def.h 2015-01-06 01:04:24.342436706 -0500
+++ b/include/linux/usb/ehci_def.h 2016-05-19 13:57:20.869304540 -0400
@@ -180,11 +180,11 @@ struct ehci_regs {
* PORTSCx
*/
/* HOSTPC: offset 0x84 */
- u32 hostpc[1]; /* HOSTPC extension */
+ u32 hostpc[0]; /* HOSTPC extension */
#define HOSTPC_PHCD (1<<22) /* Phy clock disable */
#define HOSTPC_PSPD (3<<25) /* Port speed detection */
- u32 reserved5[16];
+ u32 reserved5[17];
/* USBMODE_EX: offset 0xc8 */
u32 usbmode_ex; /* USB Device mode extension */
[toc] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2016-05-20 03:00 +0200 |
| Message-ID | <rAGXf-728-1@gated-at.bofh.it> |
| In reply to | #1404025 |
On Thu, May 19, 2016 at 05:19:00PM -0400, Valdis Kletnieks wrote:
> UBSAN throws a complaint:
>
> [ 2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
> [ 2.418582] index -1 is out of range for type 'u32 [1]'
>
> though it's only on the hostpc[] part, not on the port_status[] on the
> previous line which has the same exact index calculation. The root cause is
> that the first declaration is port_status[0], which uses a GCC extension and
> UBSAN is smart enough to realize the programmer is doing something
> intentionally odd.
>
> However, the problematic declaration is hostpc[1], which doesn't have
> the "I know what I'm doing" semantics of [0]. Change the declaration to match.
>
> Signed-Off-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
>
> --- a/include/linux/usb/ehci_def.h 2015-01-06 01:04:24.342436706 -0500
> +++ b/include/linux/usb/ehci_def.h 2016-05-19 13:57:20.869304540 -0400
> @@ -180,11 +180,11 @@ struct ehci_regs {
> * PORTSCx
> */
> /* HOSTPC: offset 0x84 */
> - u32 hostpc[1]; /* HOSTPC extension */
> + u32 hostpc[0]; /* HOSTPC extension */
> #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> #define HOSTPC_PSPD (3<<25) /* Port speed detection */
Hm, this is odd, you really do want hostpc to be 1 u32 value, don't make
it 0 please. If you walk off the end of hostpc, well, let's fix that
properly.
And are you sure this is needed? Is this a different issue from the
other long thread right now that we finally got a patch for?
thanks,
greg k-h
[toc] | [prev] | [next] | [standalone]
| From | Valdis.Kletnieks@vt.edu |
|---|---|
| Date | 2016-05-20 03:10 +0200 |
| Message-ID | <rAH6V-7kv-3@gated-at.bofh.it> |
| In reply to | #1404050 |
[Multipart message — attachments visible in raw view] — view raw
On Thu, 19 May 2016 17:50:31 -0700, Greg Kroah-Hartman said:
> On Thu, May 19, 2016 at 05:19:00PM -0400, Valdis Kletnieks wrote:
> > UBSAN throws a complaint:
> >
> > [ 2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
> > [ 2.418582] index -1 is out of range for type 'u32 [1]'
> >
> > though it's only on the hostpc[] part, not on the port_status[] on the
> > previous line which has the same exact index calculation. The root cause is
> > that the first declaration is port_status[0], which uses a GCC extension and
> > UBSAN is smart enough to realize the programmer is doing something
> > intentionally odd.
> >
> > However, the problematic declaration is hostpc[1], which doesn't have
> > the "I know what I'm doing" semantics of [0]. Change the declaration to match.
> >
> > Signed-Off-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
> >
> > --- a/include/linux/usb/ehci_def.h 2015-01-06 01:04:24.342436706 -0500
> > +++ b/include/linux/usb/ehci_def.h 2016-05-19 13:57:20.869304540 -0400
> > @@ -180,11 +180,11 @@ struct ehci_regs {
> > * PORTSCx
> > */
> > /* HOSTPC: offset 0x84 */
> > - u32 hostpc[1]; /* HOSTPC extension */
> > + u32 hostpc[0]; /* HOSTPC extension */
> > #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> > #define HOSTPC_PSPD (3<<25) /* Port speed detection */
>
> Hm, this is odd, you really do want hostpc to be 1 u32 value, don't make
> it 0 please. If you walk off the end of hostpc, well, let's fix that
> properly.
Well, UBSAN doesn't complain about the *other* use of the same exact index,
apparently because 'u32 port_status[0]' tells it to shut up we know what we're
doing.
And I'm pretty sure that if hostpc was supposed to be exactly one u32 rather
than an array, it wouldn't have the [] semantics everyplace...
[toc] | [prev] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-05-20 16:40 +0200 |
| Message-ID | <rATKO-6En-19@gated-at.bofh.it> |
| In reply to | #1404056 |
On Thu, 19 May 2016 Valdis.Kletnieks@vt.edu wrote:
> On Thu, 19 May 2016 17:50:31 -0700, Greg Kroah-Hartman said:
> > On Thu, May 19, 2016 at 05:19:00PM -0400, Valdis Kletnieks wrote:
> > > UBSAN throws a complaint:
> > >
> > > [ 2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
> > > [ 2.418582] index -1 is out of range for type 'u32 [1]'
> > >
> > > though it's only on the hostpc[] part, not on the port_status[] on the
> > > previous line which has the same exact index calculation. The root cause is
> > > that the first declaration is port_status[0], which uses a GCC extension and
> > > UBSAN is smart enough to realize the programmer is doing something
> > > intentionally odd.
> > >
> > > However, the problematic declaration is hostpc[1], which doesn't have
> > > the "I know what I'm doing" semantics of [0]. Change the declaration to match.
> > >
> > > Signed-Off-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
> > >
> > > --- a/include/linux/usb/ehci_def.h 2015-01-06 01:04:24.342436706 -0500
> > > +++ b/include/linux/usb/ehci_def.h 2016-05-19 13:57:20.869304540 -0400
> > > @@ -180,11 +180,11 @@ struct ehci_regs {
> > > * PORTSCx
> > > */
> > > /* HOSTPC: offset 0x84 */
> > > - u32 hostpc[1]; /* HOSTPC extension */
> > > + u32 hostpc[0]; /* HOSTPC extension */
> > > #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> > > #define HOSTPC_PSPD (3<<25) /* Port speed detection */
> >
> > Hm, this is odd, you really do want hostpc to be 1 u32 value, don't make
> > it 0 please. If you walk off the end of hostpc, well, let's fix that
> > properly.
>
> Well, UBSAN doesn't complain about the *other* use of the same exact index,
> apparently because 'u32 port_status[0]' tells it to shut up we know what we're
> doing.
>
> And I'm pretty sure that if hostpc was supposed to be exactly one u32 rather
> than an array, it wouldn't have the [] semantics everyplace...
hostpc is supposed to have the same number of elements as the number of
ports. (On the other hand, I'm not sure if any of the platforms which
implement the hostpc register have more than one port...)
Alan Stern
[toc] | [prev] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-05-20 16:40 +0200 |
| Message-ID | <rATKN-6En-1@gated-at.bofh.it> |
| In reply to | #1404025 |
On Thu, 19 May 2016, Valdis Kletnieks wrote:
> UBSAN throws a complaint:
>
> [ 2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
> [ 2.418582] index -1 is out of range for type 'u32 [1]'
>
> though it's only on the hostpc[] part, not on the port_status[] on the
> previous line which has the same exact index calculation. The root cause is
> that the first declaration is port_status[0], which uses a GCC extension and
> UBSAN is smart enough to realize the programmer is doing something
> intentionally odd.
>
> However, the problematic declaration is hostpc[1], which doesn't have
> the "I know what I'm doing" semantics of [0]. Change the declaration to match.
>
> Signed-Off-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
>
> --- a/include/linux/usb/ehci_def.h 2015-01-06 01:04:24.342436706 -0500
> +++ b/include/linux/usb/ehci_def.h 2016-05-19 13:57:20.869304540 -0400
> @@ -180,11 +180,11 @@ struct ehci_regs {
> * PORTSCx
> */
> /* HOSTPC: offset 0x84 */
> - u32 hostpc[1]; /* HOSTPC extension */
> + u32 hostpc[0]; /* HOSTPC extension */
> #define HOSTPC_PHCD (1<<22) /* Phy clock disable */
> #define HOSTPC_PSPD (3<<25) /* Port speed detection */
>
> - u32 reserved5[16];
> + u32 reserved5[17];
>
> /* USBMODE_EX: offset 0xc8 */
> u32 usbmode_ex; /* USB Device mode extension */
Is this problem still present with my latest patch
(http://marc.info/?l=linux-usb&m=146368979514228&w=2)?
I agree that this is a reasonable change to make in any case.
Alan Stern
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web