[PATCH 0/1] twl4030-madc: Fix arbitrarily initialized function pointer

[PATCH 0/1] twl4030-madc: Fix arbitrarily initialized function pointer

[Viktor Rosendahl]

Hi all,

I have seen this bug crash the kernel when the security framework is enabled
in the kernel config. It crashed with a bad syscall from the events/0
workqueue; probably because the security framework is initialized very early
and thus the stack has been used more [or differently] and we end up picking up
a bad [nonzero] arbitrary address from the stack.

best regards,

Viktor

[PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer

[Viktor Rosendahl]

req is an automatic variable and thus we cannot rely on it being initialized to
zero (I am leaving the 0!= NULL discussion aside). Other functions test
if this pointer is NULL, in order to determine whether it is a valid address or
not.

Signed-off-by: Viktor Rosendahl <viktor.rosendahl@nokia.com>
---
 drivers/i2c/chips/twl4030-madc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/chips/twl4030-madc.c b/drivers/i2c/chips/twl4030-madc.c
index 72b126b..743db74 100644
--- a/drivers/i2c/chips/twl4030-madc.c
+++ b/drivers/i2c/chips/twl4030-madc.c
@@ -367,6 +367,7 @@ static int twl4030_madc_ioctl(struct inode *inode, struct file *filp,
 		req.channels = (1<<par.channel);
 		req.do_avg	= par.average;
 		req.method	= TWL4030_MADC_SW1;
+		req.func_cb	= NULL;
 
 		val = twl4030_madc_conversion(&req);
 		if (val <= 0) {
-- 
1.5.5.3

Re: [PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer

[Felipe Balbi]

On Wed,  2 Jul 2008 16:36:36 +0300, Viktor Rosendahl
<viktor.rosendahl@nokia.com> wrote:
> req is an automatic variable and thus we cannot rely on it being
> initialized to
> zero (I am leaving the 0!= NULL discussion aside). Other functions test
> if this pointer is NULL, in order to determine whether it is a valid
> address or
> not.


> +		req.func_cb	= NULL;

maybe below is a better patch:

diff --git a/drivers/i2c/chips/twl4030-madc.c
b/drivers/i2c/chips/twl4030-madc.c
index 72b126b..6d8915e 100644
--- a/drivers/i2c/chips/twl4030-madc.c
+++ b/drivers/i2c/chips/twl4030-madc.c
@@ -360,7 +360,7 @@ static int twl4030_madc_ioctl(struct inode *inode,
struct file *filp,
 
        switch (cmd) {
        case TWL4030_MADC_IOCX_ADC_RAW_READ: {
-               struct twl4030_madc_request req;
+               static struct twl4030_madc_request req;
                if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
                        return -EINVAL;
-- 
Best Regards,

Felipe Balbi
http://blog.felipebalbi.com
me@felipebalbi.com

Re: [PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer

[Viktor Rosendahl]

> 
> > +		req.func_cb	= NULL;
> 
> maybe below is a better patch:
> 
> diff --git a/drivers/i2c/chips/twl4030-madc.c
> b/drivers/i2c/chips/twl4030-madc.c
> index 72b126b..6d8915e 100644
> --- a/drivers/i2c/chips/twl4030-madc.c
> +++ b/drivers/i2c/chips/twl4030-madc.c
> @@ -360,7 +360,7 @@ static int twl4030_madc_ioctl(struct inode *inode,
> struct file *filp,
>  
>         switch (cmd) {
>         case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> -               struct twl4030_madc_request req;
> +               static struct twl4030_madc_request req;
>                 if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
>                         return -EINVAL;

I don't like this idea because:

- It's fragile. This struct, which is not a const, gets initialized only
once but we are still passing a pointer to it, expecting that a fairly
complex function will not modify it. This assertion is probably true
today but makes it easier for somebody to create a bug in the future.

- You introduce another shared datum and it is only protected by the BKL
in fs/ioctl.c:vfs_ioctl().

- I didn't see any argument why this variable should be static. Making
local variables static just to get cheap zero initialization is a weird
thing to do IMO.

best regards,

Viktor

Re: [PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer

[Tony Lindgren]

* Viktor Rosendahl <Viktor.Rosendahl@nokia.com> [080703 18:37]:
> 
> > 
> > > +		req.func_cb	= NULL;
> > 
> > maybe below is a better patch:
> > 
> > diff --git a/drivers/i2c/chips/twl4030-madc.c
> > b/drivers/i2c/chips/twl4030-madc.c
> > index 72b126b..6d8915e 100644
> > --- a/drivers/i2c/chips/twl4030-madc.c
> > +++ b/drivers/i2c/chips/twl4030-madc.c
> > @@ -360,7 +360,7 @@ static int twl4030_madc_ioctl(struct inode *inode,
> > struct file *filp,
> >  
> >         switch (cmd) {
> >         case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> > -               struct twl4030_madc_request req;
> > +               static struct twl4030_madc_request req;
> >                 if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
> >                         return -EINVAL;
> 
> I don't like this idea because:
> 
> - It's fragile. This struct, which is not a const, gets initialized only
> once but we are still passing a pointer to it, expecting that a fairly
> complex function will not modify it. This assertion is probably true
> today but makes it easier for somebody to create a bug in the future.
> 
> - You introduce another shared datum and it is only protected by the BKL
> in fs/ioctl.c:vfs_ioctl().
> 
> - I didn't see any argument why this variable should be static. Making
> local variables static just to get cheap zero initialization is a weird
> thing to do IMO.

Pushing the original patch.

Tony