| Message ID | 20100322153909.GC23411@bicker (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Mon, 22 Mar 2010 15:39:50 +0000 Received: from bombadil.infradead.org [18.85.46.34] by pedra with IMAP (fetchmail-6.3.6) for <mchehab@localhost> (single-drop); Mon, 22 Mar 2010 12:40:01 -0300 (BRT) Received: from vger.kernel.org ([209.132.180.67]) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1NtjjJ-00014H-OB; Mon, 22 Mar 2010 15:39:49 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754282Ab0CVPjY (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Mon, 22 Mar 2010 11:39:24 -0400 Received: from mail-bw0-f209.google.com ([209.85.218.209]:47443 "EHLO mail-bw0-f209.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753703Ab0CVPjW (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 22 Mar 2010 11:39:22 -0400 Received: by bwz1 with SMTP id 1so2024682bwz.21 for <multiple recipients>; Mon, 22 Mar 2010 08:39:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:date:from:to:cc:subject :message-id:mail-followup-to:references:mime-version:content-type :content-disposition:in-reply-to:user-agent; bh=5bbFGY+thQwbjKjddypePdXZXQIM7ql7uRXFY7Ph8S4=; b=eWytT/hzmvQgdAd8luS9PZa+SdFnhzNWZl4UK51kDgsywi+qAKiuvhADE//Cuj7pts izGMjMTPv9OmzPTCIw1afbTcEG4Mj+0E7tbBOnLmc/HHCAIpeygnYvfoKWV4o5TqbJZN 5vksRW7moNkFM0g5s4SSZ5YDepiZSWiLLCD1U= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; b=P4eZbH6zYBNLLJ59QPMIZlYRTZj0/vyLctB+Yf1GJp5JNZBZZuTSG1S4PqhAQHgtlI CZaVPukdhwUx+usQtQRmr/cHTjnhYC8ioPfHet5aYxf8nZYg/78rAOOv3Oxm0BGMLtvQ XiCW3IvXX9Gl52aDMfyK47NXe5Cen9onbn5N4= Received: by 10.204.25.145 with SMTP id z17mr5225652bkb.181.1269272358473; Mon, 22 Mar 2010 08:39:18 -0700 (PDT) Received: from bicker ([196.43.68.63]) by mx.google.com with ESMTPS id x16sm20020236bku.23.2010.03.22.08.39.15 (version=TLSv1/SSLv3 cipher=RC4-MD5); Mon, 22 Mar 2010 08:39:17 -0700 (PDT) Date: Mon, 22 Mar 2010 18:39:09 +0300 From: Dan Carpenter <error27@gmail.com> To: Takashi Iwai <tiwai@suse.de> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>, Srinivasa Deevi <srinivasa.deevi@conexant.com>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: [patch v2] cx231xx: card->driver "Conexant cx231xx Audio" too long Message-ID: <20100322153909.GC23411@bicker> Mail-Followup-To: Dan Carpenter <error27@gmail.com>, Takashi Iwai <tiwai@suse.de>, Mauro Carvalho Chehab <mchehab@infradead.org>, Srinivasa Deevi <srinivasa.deevi@conexant.com>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org References: <20100319114957.GQ5331@bicker> <s5hr5ncxvm9.wl%tiwai@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <s5hr5ncxvm9.wl%tiwai@suse.de> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Dan Carpenter
March 22, 2010, 3:39 p.m. UTC
card->driver is 15 characters and a NULL, the original code could
cause a buffer overflow.
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
In version 2, I used a better name that Takashi Iwai suggested.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Comments
On Mon, 2010-03-22 at 18:39 +0300, Dan Carpenter wrote: > card->driver is 15 characters and a NULL, the original code could > cause a buffer overflow. > In version 2, I used a better name that Takashi Iwai suggested. Perhaps it's better to use strncpy as well. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Mon, 22 Mar 2010 08:43:47 -0700, Joe Perches wrote: > > On Mon, 2010-03-22 at 18:39 +0300, Dan Carpenter wrote: > > card->driver is 15 characters and a NULL, the original code could > > cause a buffer overflow. > > > In version 2, I used a better name that Takashi Iwai suggested. > > Perhaps it's better to use strncpy as well. strlcpy() would be safer :) But, in such a case, we want rather that the error is notified at build time. Maybe a macro like below would be helpful to catch such bugs? #define COPY_STRING(buf, src) \ do { \ if (__builtin_constant_p(src)) \ BUILD_BUG_ON(strlen(src) >= sizeof(buf)); \ strcpy(buf, src); \ } while (0) and used like: struct foo { char foo[5]; } x; COPY_STRING(x.foo, "OK"); // OK COPY_STRING(x.foo, "1234567890"); // NG Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 22, 2010 at 05:04:55PM +0100, Takashi Iwai wrote: > At Mon, 22 Mar 2010 08:43:47 -0700, > Joe Perches wrote: > > > > On Mon, 2010-03-22 at 18:39 +0300, Dan Carpenter wrote: > > > card->driver is 15 characters and a NULL, the original code could > > > cause a buffer overflow. > > > > > In version 2, I used a better name that Takashi Iwai suggested. > > > > Perhaps it's better to use strncpy as well. > > strlcpy() would be safer :) > > But, in such a case, we want rather that the error is notified at > build time. > > Maybe a macro like below would be helpful to catch such bugs? > > #define COPY_STRING(buf, src) \ > do { \ > if (__builtin_constant_p(src)) \ > BUILD_BUG_ON(strlen(src) >= sizeof(buf)); \ > strcpy(buf, src); \ > } while (0) > > and used like: > > struct foo { > char foo[5]; > } x; > > COPY_STRING(x.foo, "OK"); // OK > COPY_STRING(x.foo, "1234567890"); // NG > I can do the same thing with Smatch. The smatch check can also find bugs like this: buf = kmalloc(10, GFP_KERNEL); strcpy(buf, "1234567890"); I used smatch to find this bug and 5 others on my allmodconfig w/ staging. I also found 19 other places that use strcpy() to copy from a large buffer into a smaller buffer. Your idea is nice, but I think anyone who deliberately uses the new macro is not going to have the bug in the first place. ;) regards, dan carpenter > > Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Mon, 22 Mar 2010 19:54:30 +0300, Dan Carpenter wrote: > > On Mon, Mar 22, 2010 at 05:04:55PM +0100, Takashi Iwai wrote: > > At Mon, 22 Mar 2010 08:43:47 -0700, > > Joe Perches wrote: > > > > > > On Mon, 2010-03-22 at 18:39 +0300, Dan Carpenter wrote: > > > > card->driver is 15 characters and a NULL, the original code could > > > > cause a buffer overflow. > > > > > > > In version 2, I used a better name that Takashi Iwai suggested. > > > > > > Perhaps it's better to use strncpy as well. > > > > strlcpy() would be safer :) > > > > But, in such a case, we want rather that the error is notified at > > build time. > > > > Maybe a macro like below would be helpful to catch such bugs? > > > > #define COPY_STRING(buf, src) \ > > do { \ > > if (__builtin_constant_p(src)) \ > > BUILD_BUG_ON(strlen(src) >= sizeof(buf)); \ > > strcpy(buf, src); \ > > } while (0) > > > > and used like: > > > > struct foo { > > char foo[5]; > > } x; > > > > COPY_STRING(x.foo, "OK"); // OK > > COPY_STRING(x.foo, "1234567890"); // NG > > > > I can do the same thing with Smatch. The smatch check can also find > bugs like this: > > buf = kmalloc(10, GFP_KERNEL); > strcpy(buf, "1234567890"); > > I used smatch to find this bug and 5 others on my allmodconfig w/ staging. > I also found 19 other places that use strcpy() to copy from a large buffer > into a smaller buffer. Ah, nice. > Your idea is nice, but I think anyone who deliberately uses the new > macro is not going to have the bug in the first place. ;) Yeah, in theory, such a code should be never committed because it can be caught at build time ;) Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Mon, 22 Mar 2010 18:39:09 +0300, Dan Carpenter wrote: > > card->driver is 15 characters and a NULL, the original code could > cause a buffer overflow. > > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > In version 2, I used a better name that Takashi Iwai suggested. Acked-by: Takashi Iwai <tiwai@suse.de> Could you fix em28xx in the same way, too? thanks, Takashi > diff --git a/drivers/media/video/cx231xx/cx231xx-audio.c b/drivers/media/video/cx231xx/cx231xx-audio.c > index 7793d60..7cae95a 100644 > --- a/drivers/media/video/cx231xx/cx231xx-audio.c > +++ b/drivers/media/video/cx231xx/cx231xx-audio.c > @@ -495,7 +495,7 @@ static int cx231xx_audio_init(struct cx231xx *dev) > pcm->info_flags = 0; > pcm->private_data = dev; > strcpy(pcm->name, "Conexant cx231xx Capture"); > - strcpy(card->driver, "Conexant cx231xx Audio"); > + strcpy(card->driver, "Cx231xx-Audio"); > strcpy(card->shortname, "Cx231xx Audio"); > strcpy(card->longname, "Conexant cx231xx Audio"); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 22, 2010 at 05:04:55PM +0100, Takashi Iwai wrote: > At Mon, 22 Mar 2010 08:43:47 -0700, > Joe Perches wrote: > > > > On Mon, 2010-03-22 at 18:39 +0300, Dan Carpenter wrote: > > > card->driver is 15 characters and a NULL, the original code could > > > cause a buffer overflow. > > > > > In version 2, I used a better name that Takashi Iwai suggested. > > > > Perhaps it's better to use strncpy as well. > > strlcpy() would be safer :) > > But, in such a case, we want rather that the error is notified at > build time. > > Maybe a macro like below would be helpful to catch such bugs? > > #define COPY_STRING(buf, src) \ > do { \ > if (__builtin_constant_p(src)) \ > BUILD_BUG_ON(strlen(src) >= sizeof(buf)); \ > strcpy(buf, src); \ > } while (0) > > and used like: > > struct foo { > char foo[5]; > } x; > > COPY_STRING(x.foo, "OK"); // OK > COPY_STRING(x.foo, "1234567890"); // NG why not define strcpy this way? Marcin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/cx231xx/cx231xx-audio.c b/drivers/media/video/cx231xx/cx231xx-audio.c index 7793d60..7cae95a 100644 --- a/drivers/media/video/cx231xx/cx231xx-audio.c +++ b/drivers/media/video/cx231xx/cx231xx-audio.c @@ -495,7 +495,7 @@ static int cx231xx_audio_init(struct cx231xx *dev) pcm->info_flags = 0; pcm->private_data = dev; strcpy(pcm->name, "Conexant cx231xx Capture"); - strcpy(card->driver, "Conexant cx231xx Audio"); + strcpy(card->driver, "Cx231xx-Audio"); strcpy(card->shortname, "Cx231xx Audio"); strcpy(card->longname, "Conexant cx231xx Audio");