diff mbox

[Hardy,CVE-2010-4074,1/1] USB: serial/mos*: prevent reading uninitialized stack memory

Message ID 1295895438-1507-1-git-send-email-brad.figg@canonical.com
State Accepted
Delegated to: Stefan Bader
Headers show

Commit Message

Brad Figg Jan. 24, 2011, 6:57 p.m. UTC
From: Dan Rosenberg <drosenberg@vsecurity.com>

CVE-2010-4074

BugLink: http://bugs.launchpad.net/bugs/706149

The TIOCGICOUNT device ioctl in both mos7720.c and mos7840.c allows
unprivileged users to read uninitialized stack memory, because the
"reserved" member of the serial_icounter_struct struct declared on the
stack 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>
Cc: stable <stable@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Brad Figg <brad.figg@canonical.com>
---
 drivers/usb/serial/mos7720.c |    3 +++
 drivers/usb/serial/mos7840.c |    3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)

Comments

Tim Gardner Jan. 24, 2011, 8 p.m. UTC | #1
On 01/24/2011 11:57 AM, Brad Figg wrote:
> From: Dan Rosenberg<drosenberg@vsecurity.com>
>
> CVE-2010-4074
>
> BugLink: http://bugs.launchpad.net/bugs/706149
>
> The TIOCGICOUNT device ioctl in both mos7720.c and mos7840.c allows
> unprivileged users to read uninitialized stack memory, because the
> "reserved" member of the serial_icounter_struct struct declared on the
> stack 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>
> Cc: stable<stable@kernel.org>
> Signed-off-by: Greg Kroah-Hartman<gregkh@suse.de>
> Signed-off-by: Brad Figg<brad.figg@canonical.com>
> ---
>   drivers/usb/serial/mos7720.c |    3 +++
>   drivers/usb/serial/mos7840.c |    3 +++
>   2 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
> index e02c198..ddefce5 100644
> --- a/drivers/usb/serial/mos7720.c
> +++ b/drivers/usb/serial/mos7720.c
> @@ -1487,6 +1487,9 @@ static int mos7720_ioctl(struct usb_serial_port *port, struct file *file,
>
>   	case TIOCGICOUNT:
>   		cnow = mos7720_port->icount;
> +
> +		memset(&icount, 0, sizeof(struct serial_icounter_struct));
> +
>   		icount.cts = cnow.cts;
>   		icount.dsr = cnow.dsr;
>   		icount.rng = cnow.rng;
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index c29c912..dd1ccdd 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -2433,6 +2433,9 @@ static int mos7840_ioctl(struct usb_serial_port *port, struct file *file,
>   	case TIOCGICOUNT:
>   		cnow = mos7840_port->icount;
>   		smp_rmb();
> +
> +		memset(&icount, 0, sizeof(struct serial_icounter_struct));
> +
>   		icount.cts = cnow.cts;
>   		icount.dsr = cnow.dsr;
>   		icount.rng = cnow.rng;

Same comment as for Karmic.

Acked-by: Tim Gardner tim.gardner@canonical.com>
Stefan Bader Jan. 25, 2011, 10:47 a.m. UTC | #2
On 01/24/2011 07:57 PM, Brad Figg wrote:
> From: Dan Rosenberg <drosenberg@vsecurity.com>
> 
> CVE-2010-4074
> 
> BugLink: http://bugs.launchpad.net/bugs/706149
> 
> The TIOCGICOUNT device ioctl in both mos7720.c and mos7840.c allows
> unprivileged users to read uninitialized stack memory, because the
> "reserved" member of the serial_icounter_struct struct declared on the
> stack 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>
> Cc: stable <stable@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
As Tim said. With a bit of style differently. As mentioned in the reply to his
submissions I see upstream more and more starting to document things on the sob
area in order of process, so

(cherry-picked|backported from commit <where>)
[comment]
> Signed-off-by: Brad Figg <brad.figg@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  drivers/usb/serial/mos7720.c |    3 +++
>  drivers/usb/serial/mos7840.c |    3 +++
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
> index e02c198..ddefce5 100644
> --- a/drivers/usb/serial/mos7720.c
> +++ b/drivers/usb/serial/mos7720.c
> @@ -1487,6 +1487,9 @@ static int mos7720_ioctl(struct usb_serial_port *port, struct file *file,
>  
>  	case TIOCGICOUNT:
>  		cnow = mos7720_port->icount;
> +
> +		memset(&icount, 0, sizeof(struct serial_icounter_struct));
> +
>  		icount.cts = cnow.cts;
>  		icount.dsr = cnow.dsr;
>  		icount.rng = cnow.rng;
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index c29c912..dd1ccdd 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -2433,6 +2433,9 @@ static int mos7840_ioctl(struct usb_serial_port *port, struct file *file,
>  	case TIOCGICOUNT:
>  		cnow = mos7840_port->icount;
>  		smp_rmb();
> +
> +		memset(&icount, 0, sizeof(struct serial_icounter_struct));
> +
>  		icount.cts = cnow.cts;
>  		icount.dsr = cnow.dsr;
>  		icount.rng = cnow.rng;
Tim Gardner Jan. 25, 2011, 3:05 p.m. UTC | #3
On 01/24/2011 11:57 AM, Brad Figg wrote:
> From: Dan Rosenberg<drosenberg@vsecurity.com>
>
> CVE-2010-4074
>
> BugLink: http://bugs.launchpad.net/bugs/706149
>
> The TIOCGICOUNT device ioctl in both mos7720.c and mos7840.c allows
> unprivileged users to read uninitialized stack memory, because the
> "reserved" member of the serial_icounter_struct struct declared on the
> stack 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>
> Cc: stable<stable@kernel.org>
> Signed-off-by: Greg Kroah-Hartman<gregkh@suse.de>
> Signed-off-by: Brad Figg<brad.figg@canonical.com>
> ---
>   drivers/usb/serial/mos7720.c |    3 +++
>   drivers/usb/serial/mos7840.c |    3 +++
>   2 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
> index e02c198..ddefce5 100644
> --- a/drivers/usb/serial/mos7720.c
> +++ b/drivers/usb/serial/mos7720.c
> @@ -1487,6 +1487,9 @@ static int mos7720_ioctl(struct usb_serial_port *port, struct file *file,
>
>   	case TIOCGICOUNT:
>   		cnow = mos7720_port->icount;
> +
> +		memset(&icount, 0, sizeof(struct serial_icounter_struct));
> +
>   		icount.cts = cnow.cts;
>   		icount.dsr = cnow.dsr;
>   		icount.rng = cnow.rng;
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index c29c912..dd1ccdd 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -2433,6 +2433,9 @@ static int mos7840_ioctl(struct usb_serial_port *port, struct file *file,
>   	case TIOCGICOUNT:
>   		cnow = mos7840_port->icount;
>   		smp_rmb();
> +
> +		memset(&icount, 0, sizeof(struct serial_icounter_struct));
> +
>   		icount.cts = cnow.cts;
>   		icount.dsr = cnow.dsr;
>   		icount.rng = cnow.rng;

Applied and pushed with minor commit log edits. The advantage of adding 
the CVE number to the commit subject is that it then automatically shows 
up in the changelog. On the Cc: stable line I added the oldest kernel to 
which this patch was backported, e.g., 2.6.32.22

rtg
diff mbox

Patch

diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
index e02c198..ddefce5 100644
--- a/drivers/usb/serial/mos7720.c
+++ b/drivers/usb/serial/mos7720.c
@@ -1487,6 +1487,9 @@  static int mos7720_ioctl(struct usb_serial_port *port, struct file *file,
 
 	case TIOCGICOUNT:
 		cnow = mos7720_port->icount;
+
+		memset(&icount, 0, sizeof(struct serial_icounter_struct));
+
 		icount.cts = cnow.cts;
 		icount.dsr = cnow.dsr;
 		icount.rng = cnow.rng;
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index c29c912..dd1ccdd 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -2433,6 +2433,9 @@  static int mos7840_ioctl(struct usb_serial_port *port, struct file *file,
 	case TIOCGICOUNT:
 		cnow = mos7840_port->icount;
 		smp_rmb();
+
+		memset(&icount, 0, sizeof(struct serial_icounter_struct));
+
 		icount.cts = cnow.cts;
 		icount.dsr = cnow.dsr;
 		icount.rng = cnow.rng;