lkml.org 
[lkml]   [2008]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Fix array overflow in parport_serial.c
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


\
 
 \ /
  Last update: 2008-11-22 10:59    [W:1.564 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site