* [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory
@ 2010-09-15 21:43 Dan Rosenberg
2010-09-16 15:57 ` Steven Rostedt
2010-09-17 4:55 ` David Miller
0 siblings, 2 replies; 7+ messages in thread
From: Dan Rosenberg @ 2010-09-15 21:43 UTC (permalink / raw)
To: j.dumon; +Cc: linux-kernel, netdev, security, davem, stable
Fixed formatting (tabs and line breaks).
The TIOCGICOUNT device ioctl allows unprivileged users to read
uninitialized stack memory, because the "reserved" member of the
serial_icounter_struct struct declared on the stack in hso_get_count()
is not altered or zeroed before being copied back to the user. This
patch takes care of it.
Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com>
--- linux-2.6.35.4.orig/drivers/net/usb/hso.c 2010-08-26 19:47:12.000000000 -0400
+++ linux-2.6.35.4/drivers/net/usb/hso.c 2010-09-14 21:26:18.477585183 -0400
@@ -1653,6 +1653,8 @@ static int hso_get_count(struct hso_seri
struct uart_icount cnow;
struct hso_tiocmget *tiocmget = serial->tiocmget;
+ memset(&icount, 0, sizeof(struct serial_icounter_struct));
+
if (!tiocmget)
return -ENOENT;
spin_lock_irq(&serial->serial_lock);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory
2010-09-15 21:43 [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory Dan Rosenberg
@ 2010-09-16 15:57 ` Steven Rostedt
2010-09-16 17:07 ` Alan Cox
2010-09-17 4:55 ` David Miller
1 sibling, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2010-09-16 15:57 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: j.dumon, linux-kernel, netdev, security, davem, stable
On Wed, Sep 15, 2010 at 05:43:28PM -0400, Dan Rosenberg wrote:
> Fixed formatting (tabs and line breaks).
>
> The TIOCGICOUNT device ioctl allows unprivileged users to read
> uninitialized stack memory, because the "reserved" member of the
> serial_icounter_struct struct declared on the stack in hso_get_count()
> is not altered or zeroed before being copied back to the user. This
> patch takes care of it.
>
> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com>
>
> --- linux-2.6.35.4.orig/drivers/net/usb/hso.c 2010-08-26 19:47:12.000000000 -0400
> +++ linux-2.6.35.4/drivers/net/usb/hso.c 2010-09-14 21:26:18.477585183 -0400
> @@ -1653,6 +1653,8 @@ static int hso_get_count(struct hso_seri
> struct uart_icount cnow;
> struct hso_tiocmget *tiocmget = serial->tiocmget;
>
> + memset(&icount, 0, sizeof(struct serial_icounter_struct));
> +
Move the above to after the spinlocks.
-- Steve
> if (!tiocmget)
> return -ENOENT;
> spin_lock_irq(&serial->serial_lock);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory
2010-09-16 17:07 ` Alan Cox
@ 2010-09-16 16:52 ` Steven Rostedt
2010-09-16 17:47 ` Alan Cox
0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2010-09-16 16:52 UTC (permalink / raw)
To: Alan Cox
Cc: Dan Rosenberg, j.dumon, linux-kernel, netdev, security, davem, stable
On Thu, 2010-09-16 at 18:07 +0100, Alan Cox wrote:
> ->tiocmget;
> > >
> > > + memset(&icount, 0, sizeof(struct serial_icounter_struct));
> > > +
> >
> > Move the above to after the spinlocks.
>
> Why - its a local variable ?
>
> Easier to write
>
> struct serial_icounter_struct icount = { 0 };
>
> though
True, but if we want to micro-optimize, moving it after the spinlocks
means we do no zero initialization in the tiocmget == 0. Although gcc
may be smart enough to realize that too.
-- Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory
2010-09-16 15:57 ` Steven Rostedt
@ 2010-09-16 17:07 ` Alan Cox
2010-09-16 16:52 ` Steven Rostedt
0 siblings, 1 reply; 7+ messages in thread
From: Alan Cox @ 2010-09-16 17:07 UTC (permalink / raw)
To: Steven Rostedt
Cc: Dan Rosenberg, j.dumon, linux-kernel, netdev, security, davem, stable
->tiocmget;
> >
> > + memset(&icount, 0, sizeof(struct serial_icounter_struct));
> > +
>
> Move the above to after the spinlocks.
Why - its a local variable ?
Easier to write
struct serial_icounter_struct icount = { 0 };
though
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory
2010-09-16 17:47 ` Alan Cox
@ 2010-09-16 17:36 ` Steven Rostedt
0 siblings, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2010-09-16 17:36 UTC (permalink / raw)
To: Alan Cox
Cc: Dan Rosenberg, j.dumon, linux-kernel, netdev, security, davem, stable
On Thu, 2010-09-16 at 18:47 +0100, Alan Cox wrote:
> If you want to micro-optimise it you can enumerate the cases that
> tiocmget can be NULL in that driver..
Heh, I think that's going a bit too far. Of course mico-optimizations
usually make the system go faster, but not enough to show out of the
noise, and micro-optimizations most of the time just make the code
harder to understand and more error prone.
A lot of the patches in this patch set could just do the:
struct foo bar = { 0 };
This would let gcc optimize what needs to be done, and avoids a function
call to memset.
-- Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory
2010-09-16 16:52 ` Steven Rostedt
@ 2010-09-16 17:47 ` Alan Cox
2010-09-16 17:36 ` Steven Rostedt
0 siblings, 1 reply; 7+ messages in thread
From: Alan Cox @ 2010-09-16 17:47 UTC (permalink / raw)
To: Steven Rostedt
Cc: Dan Rosenberg, j.dumon, linux-kernel, netdev, security, davem, stable
On Thu, 16 Sep 2010 12:52:40 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 2010-09-16 at 18:07 +0100, Alan Cox wrote:
> > ->tiocmget;
> > > >
> > > > + memset(&icount, 0, sizeof(struct serial_icounter_struct));
> > > > +
> > >
> > > Move the above to after the spinlocks.
> >
> > Why - its a local variable ?
> >
> > Easier to write
> >
> > struct serial_icounter_struct icount = { 0 };
> >
> > though
>
> True, but if we want to micro-optimize, moving it after the spinlocks
> means we do no zero initialization in the tiocmget == 0. Although gcc
> may be smart enough to realize that too.
If you want to micro-optimise it you can enumerate the cases that
tiocmget can be NULL in that driver..
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory
2010-09-15 21:43 [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory Dan Rosenberg
2010-09-16 15:57 ` Steven Rostedt
@ 2010-09-17 4:55 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2010-09-17 4:55 UTC (permalink / raw)
To: drosenberg; +Cc: j.dumon, linux-kernel, netdev, security, stable
From: Dan Rosenberg <drosenberg@vsecurity.com>
Date: Wed, 15 Sep 2010 17:43:28 -0400
> Fixed formatting (tabs and line breaks).
>
> The TIOCGICOUNT device ioctl allows unprivileged users to read
> uninitialized stack memory, because the "reserved" member of the
> serial_icounter_struct struct declared on the stack in hso_get_count()
> is not altered or zeroed before being copied back to the user. This
> patch takes care of it.
>
> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-09-17 4:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-15 21:43 [PATCH v2] drivers/net/usb/hso.c: prevent reading uninitialized memory Dan Rosenberg
2010-09-16 15:57 ` Steven Rostedt
2010-09-16 17:07 ` Alan Cox
2010-09-16 16:52 ` Steven Rostedt
2010-09-16 17:47 ` Alan Cox
2010-09-16 17:36 ` Steven Rostedt
2010-09-17 4:55 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.