[PATCH 6/8] char: synclink: fix information leak to userland

[PATCH 6/8] char: synclink: fix information leak to userland

[Vasiliy Kulikov]

Structure new_line is copied to userland with some padding fields unitialized.
It leads to leaking of stack memory.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 Compile tested.

 drivers/char/synclink.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/char/synclink.c b/drivers/char/synclink.c
index 3a6824f..abd0867 100644
--- a/drivers/char/synclink.c
+++ b/drivers/char/synclink.c
@@ -7846,6 +7846,8 @@ static int hdlcdev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	if (cmd != SIOCWANDEV)
 		return hdlc_ioctl(dev, ifr, cmd);
 
+	memset(&new_line, 0, size);
+
 	switch(ifr->ifr_settings.type) {
 	case IF_GET_IFACE: /* return current sync_serial_settings */
 
-- 
1.7.0.4

Re: [PATCH 6/8] char: synclink: fix information leak to userland

[Jiri Slaby]

On 10/17/2010 04:41 PM, Vasiliy Kulikov wrote:
> Structure new_line is copied to userland with some padding fields unitialized.
> It leads to leaking of stack memory.

I think your tool has a bug. I must admit I fail to see the padding
which would cause leaks. Could you elaborate?

> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  Compile tested.
> 
>  drivers/char/synclink.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/synclink.c b/drivers/char/synclink.c
> index 3a6824f..abd0867 100644
> --- a/drivers/char/synclink.c
> +++ b/drivers/char/synclink.c
> @@ -7846,6 +7846,8 @@ static int hdlcdev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>  	if (cmd != SIOCWANDEV)
>  		return hdlc_ioctl(dev, ifr, cmd);
>  
> +	memset(&new_line, 0, size);
> +
>  	switch(ifr->ifr_settings.type) {
>  	case IF_GET_IFACE: /* return current sync_serial_settings */
>  


-- 
js
suse labs

Re: [PATCH 6/8] char: synclink: fix information leak to userland

[Vasiliy Kulikov]

On Sun, Oct 17, 2010 at 17:34 +0200, Jiri Slaby wrote:
> On 10/17/2010 04:41 PM, Vasiliy Kulikov wrote:
> > Structure new_line is copied to userland with some padding fields unitialized.
> > It leads to leaking of stack memory.
> 
> I think your tool has a bug. I must admit I fail to see the padding
> which would cause leaks. Could you elaborate?

I didn't use any tool except "grep copy_to_user" :)

    typedef struct { 
        unsigned int clock_rate; /* bits per second */
        unsigned int clock_type; /* internal, external, TX-internal etc. */
        unsigned short loopback;
    } sync_serial_settings;          /* V.35, V.24, X.21 */

On x86_64 sizeof(sync_serial_settings) = 4 + 4 + 2 + 2 = 12.
The last 2 is padding.

> 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
> >  Compile tested.
> > 
> >  drivers/char/synclink.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/char/synclink.c b/drivers/char/synclink.c
> > index 3a6824f..abd0867 100644
> > --- a/drivers/char/synclink.c
> > +++ b/drivers/char/synclink.c
> > @@ -7846,6 +7846,8 @@ static int hdlcdev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> >  	if (cmd != SIOCWANDEV)
> >  		return hdlc_ioctl(dev, ifr, cmd);
> >  
> > +	memset(&new_line, 0, size);
> > +
> >  	switch(ifr->ifr_settings.type) {
> >  	case IF_GET_IFACE: /* return current sync_serial_settings */
> >  
> 
> 
> -- 
> js
> suse labs

-- 
Vasiliy

Re: [PATCH 6/8] char: synclink: fix information leak to userland

[Dan Carpenter]

On Sun, Oct 17, 2010 at 07:38:39PM +0400, Vasiliy Kulikov wrote:
> On Sun, Oct 17, 2010 at 17:34 +0200, Jiri Slaby wrote:
> > On 10/17/2010 04:41 PM, Vasiliy Kulikov wrote:
> > > Structure new_line is copied to userland with some padding fields unitialized.
> > > It leads to leaking of stack memory.
> > 
> > I think your tool has a bug. I must admit I fail to see the padding
> > which would cause leaks. Could you elaborate?
> 
> I didn't use any tool except "grep copy_to_user" :)
> 

It seems like you should be able to use pahole to make a list of
structs with padding and then a checker script to find places where
information is leaked.

Also someone complained to me about when I added a memset() in a fast
path.  The thought was that it might be faster to just initialize it
instead like:

	struct foo bar = {};

In my case just using the initializer made the code cleaner so I did it,
but neither of us actually benchmarked it.

regards,
dan carpenter

Re: [PATCH 6/8] char: synclink: fix information leak to userland

[Vasiliy Kulikov]

On Sun, Oct 17, 2010 at 19:36 +0200, Dan Carpenter wrote:
> On Sun, Oct 17, 2010 at 07:38:39PM +0400, Vasiliy Kulikov wrote:
> > On Sun, Oct 17, 2010 at 17:34 +0200, Jiri Slaby wrote:
> > > On 10/17/2010 04:41 PM, Vasiliy Kulikov wrote:
> > > > Structure new_line is copied to userland with some padding fields unitialized.
> > > > It leads to leaking of stack memory.
> > > 
> > > I think your tool has a bug. I must admit I fail to see the padding
> > > which would cause leaks. Could you elaborate?
> > 
> > I didn't use any tool except "grep copy_to_user" :)
> > 
> 
> It seems like you should be able to use pahole to make a list of
> structs with padding and then a checker script to find places where
> information is leaked.

Not all of these patches fix only padding zeroing, some of them fix
uninitialized fields.  One struct has partly initialized array.

> Also someone complained to me about when I added a memset() in a fast
> path.

All these cases are ioctl() handlers or similar.  I don't think ioctl()
should be so fast to become significantly slower with single memset().

> The thought was that it might be faster to just initialize it
> instead like:
> 
> 	struct foo bar = {};
> 
> In my case just using the initializer made the code cleaner so I did it,
> but neither of us actually benchmarked it.
> 
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Thanks,

-- 
Vasiliy

Re: [PATCH 6/8] char: synclink: fix information leak to userland

[Jiri Slaby]

On 10/17/2010 05:38 PM, Vasiliy Kulikov wrote:
> On Sun, Oct 17, 2010 at 17:34 +0200, Jiri Slaby wrote:
>> On 10/17/2010 04:41 PM, Vasiliy Kulikov wrote:
>>> Structure new_line is copied to userland with some padding fields unitialized.
>>> It leads to leaking of stack memory.
>>
>> I think your tool has a bug. I must admit I fail to see the padding
>> which would cause leaks. Could you elaborate?
> 
> I didn't use any tool except "grep copy_to_user" :)
> 
>     typedef struct { 
>         unsigned int clock_rate; /* bits per second */
>         unsigned int clock_type; /* internal, external, TX-internal etc. */
>         unsigned short loopback;
>     } sync_serial_settings;          /* V.35, V.24, X.21 */
> 
> On x86_64 sizeof(sync_serial_settings) = 4 + 4 + 2 + 2 = 12.
> The last 2 is padding.

Ah, good to know that even end of structures is padded.

thanks for clarification,
-- 
js
suse labs

Re: [PATCH 6/8] char: synclink: fix information leak to userland

[Vasiliy Kulikov]

On Sun, Oct 17, 2010 at 19:46 +0200, Jiri Slaby wrote:
> On 10/17/2010 05:38 PM, Vasiliy Kulikov wrote:
> > On Sun, Oct 17, 2010 at 17:34 +0200, Jiri Slaby wrote:
> >> On 10/17/2010 04:41 PM, Vasiliy Kulikov wrote:
> >>> Structure new_line is copied to userland with some padding fields unitialized.
> >>> It leads to leaking of stack memory.
> >>
> >> I think your tool has a bug. I must admit I fail to see the padding
> >> which would cause leaks. Could you elaborate?
> > 
> > I didn't use any tool except "grep copy_to_user" :)
> > 
> >     typedef struct { 
> >         unsigned int clock_rate; /* bits per second */
> >         unsigned int clock_type; /* internal, external, TX-internal etc. */
> >         unsigned short loopback;
> >     } sync_serial_settings;          /* V.35, V.24, X.21 */
> > 
> > On x86_64 sizeof(sync_serial_settings) = 4 + 4 + 2 + 2 = 12.
> > The last 2 is padding.
> 
> Ah, good to know that even end of structures is padded.

The rule here is that array of struct has no padding between struct
elements.  So, struct has padding in the end to have good alignment of
the first field of the next array element.

Thanks,

-- 
Vasiliy