[patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Dan Carpenter]

"adapter" is used as an array index in the adapters[] array so
the off by one would make us read past the end.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
1c073b67979 "ALSA: asihpi - Remove spurious adapter index check"
reverted Dan Rosenburg's check that would have prevented the
overflow here.

Also it moved the initialization of "pa" down a couple lines so I'm
concerned there may be a bogus derereference here when we check
pa->type.  I don't have the hardware, so I can't test this.

diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
index 65fcf47..7ba7073 100644
--- a/sound/pci/asihpi/hpioctl.c
+++ b/sound/pci/asihpi/hpioctl.c
@@ -183,7 +183,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		int wrflag = -1;
 		u32 adapter = hm->h.adapter_index;
 
-		if ((adapter > HPI_MAX_ADAPTERS) || (!pa->type)) {
+		if ((adapter >= HPI_MAX_ADAPTERS) || (!pa->type)) {
 			hpi_init_response(&hr->r0, HPI_OBJ_ADAPTER,
 				HPI_ADAPTER_OPEN,
 				HPI_ERROR_BAD_ADAPTER_NUMBER);

[patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Dan Carpenter]

"adapter" is used as an array index in the adapters[] array so
the off by one would make us read past the end.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
1c073b67979 "ALSA: asihpi - Remove spurious adapter index check"
reverted Dan Rosenburg's check that would have prevented the
overflow here.

Also it moved the initialization of "pa" down a couple lines so I'm
concerned there may be a bogus derereference here when we check
pa->type.  I don't have the hardware, so I can't test this.

diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
index 65fcf47..7ba7073 100644
--- a/sound/pci/asihpi/hpioctl.c
+++ b/sound/pci/asihpi/hpioctl.c
@@ -183,7 +183,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		int wrflag = -1;
 		u32 adapter = hm->h.adapter_index;
 
-		if ((adapter > HPI_MAX_ADAPTERS) || (!pa->type)) {
+		if ((adapter >= HPI_MAX_ADAPTERS) || (!pa->type)) {
 			hpi_init_response(&hr->r0, HPI_OBJ_ADAPTER,
 				HPI_ADAPTER_OPEN,
 				HPI_ERROR_BAD_ADAPTER_NUMBER);

Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Dan Rosenberg]

On Wed, 2011-07-27 at 15:02 +0300, Dan Carpenter wrote:
> "adapter" is used as an array index in the adapters[] array so
> the off by one would make us read past the end.
> 

Agreed. I also don't like the fact that the "pa" pointer can be set to
an arbitrary address because the index isn't checked until after its
assignment. Even though the fix to the check prevents this pointer from
being dereferenced if it's out-of-bounds, it's still wrong.

> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> 1c073b67979 "ALSA: asihpi - Remove spurious adapter index check"
> reverted Dan Rosenburg's check that would have prevented the
> overflow here.
> 

If it's going in the commit log, s/Rosenburg/Rosenberg please. :-)

> Also it moved the initialization of "pa" down a couple lines so I'm
> concerned there may be a bogus derereference here when we check
> pa->type.  I don't have the hardware, so I can't test this.
> 

I agree.  This code seems to make assumptions in more than one place
that the adapters array is fully populated with non-NULL elements.  At a
glance, I can't see where such initialization occurs though.

> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 65fcf47..7ba7073 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -183,7 +183,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		int wrflag = -1;
>  		u32 adapter = hm->h.adapter_index;
>  
> -		if ((adapter > HPI_MAX_ADAPTERS) || (!pa->type)) {
> +		if ((adapter >= HPI_MAX_ADAPTERS) || (!pa->type)) {
>  			hpi_init_response(&hr->r0, HPI_OBJ_ADAPTER,
>  				HPI_ADAPTER_OPEN,
>  				HPI_ERROR_BAD_ADAPTER_NUMBER);

Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Dan Rosenberg]

> > Also it moved the initialization of "pa" down a couple lines so I'm
> > concerned there may be a bogus derereference here when we check
> > pa->type.  I don't have the hardware, so I can't test this.
> > 
> 
> I agree.  This code seems to make assumptions in more than one place
> that the adapters array is fully populated with non-NULL elements.  At a
> glance, I can't see where such initialization occurs though.
> 

I hadn't read the updated code fully, so I missed what you meant.  Yes,
this is definitely a NULL dereference, since "pa" is initialized to NULL
and not changed until after this dereference.

> > diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> > index 65fcf47..7ba7073 100644
> > --- a/sound/pci/asihpi/hpioctl.c
> > +++ b/sound/pci/asihpi/hpioctl.c
> > @@ -183,7 +183,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >  		int wrflag = -1;
> >  		u32 adapter = hm->h.adapter_index;
> >  
> > -		if ((adapter > HPI_MAX_ADAPTERS) || (!pa->type)) {
> > +		if ((adapter >= HPI_MAX_ADAPTERS) || (!pa->type)) {
> >  			hpi_init_response(&hr->r0, HPI_OBJ_ADAPTER,
> >  				HPI_ADAPTER_OPEN,
> >  				HPI_ERROR_BAD_ADAPTER_NUMBER);
> 

Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Takashi Iwai]

At Wed, 27 Jul 2011 09:02:13 -0400,
Dan Rosenberg wrote:
> 
> 
> > > Also it moved the initialization of "pa" down a couple lines so I'm
> > > concerned there may be a bogus derereference here when we check
> > > pa->type.  I don't have the hardware, so I can't test this.
> > > 
> > 
> > I agree.  This code seems to make assumptions in more than one place
> > that the adapters array is fully populated with non-NULL elements.  At a
> > glance, I can't see where such initialization occurs though.
> > 
> 
> I hadn't read the updated code fully, so I missed what you meant.  Yes,
> this is definitely a NULL dereference, since "pa" is initialized to NULL
> and not changed until after this dereference.

NULL dereference was already fixed today in sound git tree by commit
767cd365b22820df07b962b49ce04b220b98e537.

It'll be included in the pull request in the next days, maybe
tomorrow.


thanks,

Takashi

Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Takashi Iwai]

At Wed, 27 Jul 2011 09:02:13 -0400,
Dan Rosenberg wrote:
> 
> 
> > > Also it moved the initialization of "pa" down a couple lines so I'm
> > > concerned there may be a bogus derereference here when we check
> > > pa->type.  I don't have the hardware, so I can't test this.
> > > 
> > 
> > I agree.  This code seems to make assumptions in more than one place
> > that the adapters array is fully populated with non-NULL elements.  At a
> > glance, I can't see where such initialization occurs though.
> > 
> 
> I hadn't read the updated code fully, so I missed what you meant.  Yes,
> this is definitely a NULL dereference, since "pa" is initialized to NULL
> and not changed until after this dereference.

NULL dereference was already fixed today in sound git tree by commit
767cd365b22820df07b962b49ce04b220b98e537.

It'll be included in the pull request in the next days, maybe
tomorrow.


thanks,

Takashi

Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Takashi Iwai]

At Wed, 27 Jul 2011 15:02:26 +0300,
Dan Carpenter wrote:
> 
> "adapter" is used as an array index in the adapters[] array so
> the off by one would make us read past the end.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Applied now.  Thanks.


Takashi

> ---
> 1c073b67979 "ALSA: asihpi - Remove spurious adapter index check"
> reverted Dan Rosenburg's check that would have prevented the
> overflow here.
> 
> Also it moved the initialization of "pa" down a couple lines so I'm
> concerned there may be a bogus derereference here when we check
> pa->type.  I don't have the hardware, so I can't test this.
> 
> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 65fcf47..7ba7073 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -183,7 +183,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		int wrflag = -1;
>  		u32 adapter = hm->h.adapter_index;
>  
> -		if ((adapter > HPI_MAX_ADAPTERS) || (!pa->type)) {
> +		if ((adapter >= HPI_MAX_ADAPTERS) || (!pa->type)) {
>  			hpi_init_response(&hr->r0, HPI_OBJ_ADAPTER,
>  				HPI_ADAPTER_OPEN,
>  				HPI_ERROR_BAD_ADAPTER_NUMBER);
> 

Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Takashi Iwai]

At Wed, 27 Jul 2011 15:02:26 +0300,
Dan Carpenter wrote:
> 
> "adapter" is used as an array index in the adapters[] array so
> the off by one would make us read past the end.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Applied now.  Thanks.


Takashi

> ---
> 1c073b67979 "ALSA: asihpi - Remove spurious adapter index check"
> reverted Dan Rosenburg's check that would have prevented the
> overflow here.
> 
> Also it moved the initialization of "pa" down a couple lines so I'm
> concerned there may be a bogus derereference here when we check
> pa->type.  I don't have the hardware, so I can't test this.
> 
> diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
> index 65fcf47..7ba7073 100644
> --- a/sound/pci/asihpi/hpioctl.c
> +++ b/sound/pci/asihpi/hpioctl.c
> @@ -183,7 +183,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		int wrflag = -1;
>  		u32 adapter = hm->h.adapter_index;
>  
> -		if ((adapter > HPI_MAX_ADAPTERS) || (!pa->type)) {
> +		if ((adapter >= HPI_MAX_ADAPTERS) || (!pa->type)) {
>  			hpi_init_response(&hr->r0, HPI_OBJ_ADAPTER,
>  				HPI_ADAPTER_OPEN,
>  				HPI_ERROR_BAD_ADAPTER_NUMBER);
> 

Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Eliot Blennerhassett]

On 28/07/11 00:55, Dan Rosenberg wrote:
> On Wed, 2011-07-27 at 15:02 +0300, Dan Carpenter wrote:
>> "adapter" is used as an array index in the adapters[] array so
>> the off by one would make us read past the end.
>>
> 
> Agreed. I also don't like the fact that the "pa" pointer can be set to
> an arbitrary address because the index isn't checked until after its
> assignment. Even though the fix to the check prevents this pointer from
> being dereferenced if it's out-of-bounds, it's still wrong.

:( I have to agree.
I'll submit another patch that avoids doing this.

> I agree.  This code seems to make assumptions in more than one place
> that the adapters array is fully populated with non-NULL elements.  At a
> glance, I can't see where such initialization occurs though.

It does, in void __init asihpi_init(void)
...	memset(adapters, 0, sizeof(adapters));

-- 
Eliot Blennerhassett
AudioScience Inc.

Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()

[Eliot Blennerhassett]

On 28/07/11 00:55, Dan Rosenberg wrote:
> On Wed, 2011-07-27 at 15:02 +0300, Dan Carpenter wrote:
>> "adapter" is used as an array index in the adapters[] array so
>> the off by one would make us read past the end.
>>
> 
> Agreed. I also don't like the fact that the "pa" pointer can be set to
> an arbitrary address because the index isn't checked until after its
> assignment. Even though the fix to the check prevents this pointer from
> being dereferenced if it's out-of-bounds, it's still wrong.

:( I have to agree.
I'll submit another patch that avoids doing this.

> I agree.  This code seems to make assumptions in more than one place
> that the adapters array is fully populated with non-NULL elements.  At a
> glance, I can't see where such initialization occurs though.

It does, in void __init asihpi_init(void)
...	memset(adapters, 0, sizeof(adapters));

-- 
Eliot Blennerhassett
AudioScience Inc.