usb: core: fix information leak to userland
Subsystems (2)
PATCH 1/1 usb: core: fix information leak to userland
Structure usbdevfs_connectinfo is copied to userland with padding byted after "slow" field uninitialized. It leads to leaking of contents of kernel stack memory.
---
Compile tested.
drivers/usb/core/devio.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
drivers/usb/core/devio.c +4 -3
@@ -965,10 +965,11 @@ static int proc_getdriver(struct dev_state *ps, void __user *arg)static int proc_connectinfo (struct dev_state *ps,void __user *arg) { -struct usbdevfs_connectinfo ci; +struct usbdevfs_connectinfo ci = { + .devnum = ps->dev->devnum, + .slow = ps->dev->speed =USB_SPEED_LOW + }; - ci.devnum = ps->dev->devnum; - ci.slow = ps->dev->speed =USB_SPEED_LOW ;if (copy_to_user (arg, &ci,sizeof (ci)))return -EFAULT ;return 0 ;
Signature · 1 line
1.7.0.4
Quoted 20 lines · Vasiliy Kulikov
Structure
usbdevfs_connectinfois copied to userland with padding byted after "slow" field uninitialized. It leads to leaking of contents of kernel stack memory.Signed-off-by Vasiliy Kulikov <[email protected]>---
Compile tested.drivers/usb/core/devio.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)drivers/usb/core/devio.c +0
-1@@ -965,10 +965,11 @@ static int proc_getdriver(struct dev_state *ps, void __user *arg)static int proc_connectinfo (struct dev_state *ps,void __user *arg) { -struct usbdevfs_connectinfo ci;
drivers/usb/core/devio.c static int proc_getdriver(struct dev_state *ps, void __user *arg) · line 975
Quoted 10 lines
drivers/usb/core/devio.c +4
-2+struct usbdevfs_connectinfo ci = { + .devnum = ps->dev->devnum, + .slow = ps->dev->speed =USB_SPEED_LOW + }; - ci.devnum = ps->dev->devnum; - ci.slow = ps->dev->speed =USB_SPEED_LOW ;if (copy_to_user (arg, &ci,sizeof (ci)))return -EFAULT ;return 0 ;
Are you sure that adding an initializer like this will zero out the padding bytes? It might be safer just to call memset. This is not exactly a hot path, after all.
Alan Stern
Quoted 4 lines · Alan Stern
Are you sure that adding an initializer
like this will zero out the
padding bytes? It might be safer just to call
memset.
ISTR the C standard says things get initted to
zero in this case too ... and that compilers will
as a rule use memset to do it. One could look
at the generated code to make sure of that.
There's certainly a fair amount of code I've seen
that uses runtime initializers like that, to zero
memory. I can't believe i's _all_ broken! ;)
- Dave
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · David Brownell
Quoted 4 lines · Alan Stern
Are you sure that adding an initializer
like this will zero out the
padding bytes? It might be safer just to call
memset.ISTR the C standard says things get initted to zero in this case too ... and that compilers will as a rule use memset to do it. One could look at the generated code to make sure of that.
Unfortunately I don't have a copy of the C standard here to consult. However... Although I'm perfectly willing to believe that the standard requires fields in a structure to be initialized to 0 if they aren't mentioned explicitly in the initializer, I'm considerably more doubtful that it also requires padding to be initialized!
And I certainly wouldn't want to depend on compilers _always_ using memset to do this initialization.
Quoted 3 lines
There's certainly a fair amount of code I've seen that uses runtime initializers like that, to zero memory. I can't believe i's _all_ broken! ;)
Zeroing memory that belongs to a declared field is different from zeroing padding bytes. Maybe what you remember seeing is the first and not the second.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · Alan Stern
Quoted 1 line
Unfortunately I don't have a copy of the C standard here to consult.
However... Although I'm perfectly willing to believe that the standard
requires fields in a structure to be initialized to 0 if they
aren't mentioned explicitly in the initializer, I'm considerably more
doubtful that it also requires padding to be initialized!
ISTR initialization-to-zero is the standard behavior defined for all _memory_ that gets initialized ... not just named fields ... whether the init is "static", "bss", or not.
Quoted 4 lines
And I certainly wouldn't want to depend on compilers _always_ using
memset to do this initialization.
Of course not; just rely on init-to-zero, and let the compiler worry about efficiency. In some cases memset(); in others, the result might be as if memset were inlined, so only a few "write a zero at this address" type instructions would be needed.
Quoted 11 lines
Quoted 4 lines
There's certainly a fair amount of code I've seen
that uses runtime initializers like that, to zero
memory. I can't believe i's _all_ broken!;)
Zeroing memory that belongs to a declared field is different from
zeroing padding bytes. Maybe what you remember seeing is the first and
not the second.
I remember seeing both, and at one point looking at the issue to verify that padding was treated uniformly (like other memory). Also, writing code that relied on zero-initted padding.
- Dave
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html