[PATCH] Fix array overflow in parport_serial.c

[PATCH] Fix array overflow in parport_serial.c

[Takashi Iwai]

The netmos_9xx5_combo type assumes that PCI SSID provides always the
correct value for the number of parallel and serial ports, but there
are indeed broken devices with wrong numbers, which may result in
Oops.

This patch simply adds the check of the array range.

Reference: Novell bnc#447067
	https://bugzilla.novell.com/show_bug.cgi?id=447067

Signed-off-by: Takashi Iwai <tiwai@suse.de>

---
diff --git a/drivers/parport/parport_serial.c b/drivers/parport/parport_serial.c
index e2e95b3..101ed49 100644
--- a/drivers/parport/parport_serial.c
+++ b/drivers/parport/parport_serial.c
@@ -70,6 +70,8 @@ static int __devinit netmos_parallel_init(struct pci_dev *dev, struct parport_pc
 	 * parallel ports and <S> is the number of serial ports.
 	 */
 	card->numports = (dev->subsystem_device & 0xf0) >> 4;
+	if (card->numports > ARRAY_SIZE(card->addr))
+		card->numports = ARRAY_SIZE(card->addr);
 	return 0;
 }
 

Re: [PATCH] Fix array overflow in parport_serial.c

[Andrew Morton]

On Thu, 20 Nov 2008 17:35:20 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> Subject: [PATCH] Fix array overflow in parport_serial.c

Please prefer titles in the form

	subsystem identifer: what was done to it

I renamed this one to

	parport_serial: fix array overflow

> Date: Thu, 20 Nov 2008 17:35:20 +0100
> User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka)
>  FLIM/1.14.7 (Sanj__) APEL/10.6 Emacs/22.3
>  (x86_64-suse-linux-gnu) MULE/5.0 (SAKAKI)
> 
> The netmos_9xx5_combo type assumes that PCI SSID provides always the
> correct value for the number of parallel and serial ports, but there
> are indeed broken devices with wrong numbers, which may result in
> Oops.
> 
> This patch simply adds the check of the array range.
> 
> Reference: Novell bnc#447067
> 	https://bugzilla.novell.com/show_bug.cgi?id=447067
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> ---
> diff --git a/drivers/parport/parport_serial.c b/drivers/parport/parport_serial.c
> index e2e95b3..101ed49 100644
> --- a/drivers/parport/parport_serial.c
> +++ b/drivers/parport/parport_serial.c
> @@ -70,6 +70,8 @@ static int __devinit netmos_parallel_init(struct pci_dev *dev, struct parport_pc
>  	 * parallel ports and <S> is the number of serial ports.
>  	 */
>  	card->numports = (dev->subsystem_device & 0xf0) >> 4;
> +	if (card->numports > ARRAY_SIZE(card->addr))

hm.  ARRAY_SIZE returns an unsigned type so we don't have to worry
about negative values when doing comparisons like this.  Not that
card->numports could be negative anyway, but it's always nice to set
readers' minds at rest..

> +		card->numports = ARRAY_SIZE(card->addr);
>  	return 0;
>  }


Should we emit some kind of warning when this is detected?  I guess
not, if we're sure that there will never be a situation in which users
find that some of their ports don't work?

Re: [PATCH] Fix array overflow in parport_serial.c

[Takashi Iwai]

At Fri, 21 Nov 2008 14:16:21 -0800,
Andrew Morton wrote:
> 
> On Thu, 20 Nov 2008 17:35:20 +0100
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > Subject: [PATCH] Fix array overflow in parport_serial.c
> 
> Please prefer titles in the form
> 
> 	subsystem identifer: what was done to it
> 
> I renamed this one to
> 
> 	parport_serial: fix array overflow

Yep, better.  Thanks.

> > Date: Thu, 20 Nov 2008 17:35:20 +0100
> > User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka)
> >  FLIM/1.14.7 (Sanj__) APEL/10.6 Emacs/22.3
> >  (x86_64-suse-linux-gnu) MULE/5.0 (SAKAKI)
> > 
> > The netmos_9xx5_combo type assumes that PCI SSID provides always the
> > correct value for the number of parallel and serial ports, but there
> > are indeed broken devices with wrong numbers, which may result in
> > Oops.
> > 
> > This patch simply adds the check of the array range.
> > 
> > Reference: Novell bnc#447067
> > 	https://bugzilla.novell.com/show_bug.cgi?id=447067
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > ---
> > diff --git a/drivers/parport/parport_serial.c b/drivers/parport/parport_serial.c
> > index e2e95b3..101ed49 100644
> > --- a/drivers/parport/parport_serial.c
> > +++ b/drivers/parport/parport_serial.c
> > @@ -70,6 +70,8 @@ static int __devinit netmos_parallel_init(struct pci_dev *dev, struct parport_pc
> >  	 * parallel ports and <S> is the number of serial ports.
> >  	 */
> >  	card->numports = (dev->subsystem_device & 0xf0) >> 4;
> > +	if (card->numports > ARRAY_SIZE(card->addr))
> 
> hm.  ARRAY_SIZE returns an unsigned type so we don't have to worry
> about negative values when doing comparisons like this.  Not that
> card->numports could be negative anyway, but it's always nice to set
> readers' minds at rest..

I think changing numports to unsigned is the easiest way.

> > +		card->numports = ARRAY_SIZE(card->addr);
> >  	return 0;
> >  }
> 
> 
> Should we emit some kind of warning when this is detected?  I guess
> not, if we're sure that there will never be a situation in which users
> find that some of their ports don't work?

I thought of that, too, but wanted to make the change minimum.

Besides that, I'm not sure whether netmos_parallel_init() really works
as its description.  It changes numports, but it doesn't change the
addr[].  In cards[], only one item is defined, thus all of the reset
point BAR 0.

I made the patch just to fix the oops, but this code should be fixed
in a better way, anyway.


thanks,

Takashi