From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756770AbYKVJ4T (ORCPT ); Sat, 22 Nov 2008 04:56:19 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752738AbYKVJ4K (ORCPT ); Sat, 22 Nov 2008 04:56:10 -0500 Received: from ns1.suse.de ([195.135.220.2]:56199 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752032AbYKVJ4J (ORCPT ); Sat, 22 Nov 2008 04:56:09 -0500 Date: Sat, 22 Nov 2008 10:56:05 +0100 Message-ID: From: Takashi Iwai To: Andrew Morton Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] Fix array overflow in parport_serial.c In-Reply-To: <20081121141621.fe4277bb.akpm@linux-foundation.org> References: <20081121141621.fe4277bb.akpm@linux-foundation.org> User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka) FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 Emacs/22.3 (x86_64-suse-linux-gnu) MULE/5.0 (SAKAKI) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Fri, 21 Nov 2008 14:16:21 -0800, Andrew Morton wrote: > > On Thu, 20 Nov 2008 17:35:20 +0100 > Takashi Iwai 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 > > > > --- > > 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 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