* [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
@ 2011-07-27 12:02 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2011-07-27 12:02 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: Takashi Iwai, Eliot Blennerhassett, Dan Rosenberg,
kernel-janitors, open list:SOUND
"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);
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
2011-07-27 12:02 ` Dan Carpenter
(?)
@ 2011-07-27 12:55 ` Dan Rosenberg
2011-07-27 21:45 ` Eliot Blennerhassett
-1 siblings, 1 reply; 10+ messages in thread
From: Dan Rosenberg @ 2011-07-27 12:55 UTC (permalink / raw)
To: kernel-janitors
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);
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
2011-07-27 12:55 ` Dan Rosenberg
@ 2011-07-27 21:45 ` Eliot Blennerhassett
0 siblings, 0 replies; 10+ messages in thread
From: Eliot Blennerhassett @ 2011-07-27 21:45 UTC (permalink / raw)
To: Dan Rosenberg
Cc: Takashi Iwai, open list:SOUND, kernel-janitors, Dan Carpenter
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
@ 2011-07-27 21:45 ` Eliot Blennerhassett
0 siblings, 0 replies; 10+ messages in thread
From: Eliot Blennerhassett @ 2011-07-27 21:45 UTC (permalink / raw)
To: Dan Rosenberg
Cc: Takashi Iwai, open list:SOUND, kernel-janitors, Dan Carpenter
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
2011-07-27 12:02 ` Dan Carpenter
(?)
(?)
@ 2011-07-27 13:02 ` Dan Rosenberg
2011-07-27 13:06 ` Takashi Iwai
-1 siblings, 1 reply; 10+ messages in thread
From: Dan Rosenberg @ 2011-07-27 13:02 UTC (permalink / raw)
To: kernel-janitors
> > 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);
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
2011-07-27 13:02 ` Dan Rosenberg
@ 2011-07-27 13:06 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2011-07-27 13:06 UTC (permalink / raw)
To: Dan Rosenberg
Cc: Eliot Blennerhassett, open list:SOUND, kernel-janitors,
Dan Carpenter
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
@ 2011-07-27 13:06 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2011-07-27 13:06 UTC (permalink / raw)
To: Dan Rosenberg
Cc: Eliot Blennerhassett, open list:SOUND, kernel-janitors,
Dan Carpenter
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
2011-07-27 12:02 ` Dan Carpenter
@ 2011-07-27 14:05 ` Takashi Iwai
-1 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2011-07-27 14:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Eliot Blennerhassett, Dan Rosenberg, kernel-janitors,
open list:SOUND
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);
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch] ALSA: asihpi - off by one in asihpi_hpi_ioctl()
@ 2011-07-27 14:05 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2011-07-27 14:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Eliot Blennerhassett, Dan Rosenberg, kernel-janitors,
open list:SOUND
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);
>
^ permalink raw reply [flat|nested] 10+ messages in thread