linux-kernel - Re: [PATCH] staging: line6: fix possible overrun

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140426215945.GC17562@mguzik.redhat.com>
Date:	Sat, 26 Apr 2014 23:59:46 +0200
From:	Mateusz Guzik <mguzik@...hat.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Laurent Navet <laurent.navet@...il.com>, tiwai@...e.de,
	gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: line6: fix possible overrun

On Sun, Apr 27, 2014 at 12:36:21AM +0300, Dan Carpenter wrote:
> On Sat, Apr 26, 2014 at 10:47:05PM +0200, Mateusz Guzik wrote:
> > On Sat, Apr 26, 2014 at 07:09:22PM +0200, Laurent Navet wrote:
> > > The strcpy operation may write past the end of the fixed-size destination
> > > buffer if the source buffer is too large.
> > > 
> > > Found by coverity scan : CID 144979
> > > 
> > > Signed-off-by: Laurent Navet <laurent.navet@...il.com>
> > > ---
> > > build tested only
> > > 
> > >  drivers/staging/line6/audio.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/line6/audio.c b/drivers/staging/line6/audio.c
> > > index 171d80c..65f5cd4 100644
> > > --- a/drivers/staging/line6/audio.c
> > > +++ b/drivers/staging/line6/audio.c
> > > @@ -32,9 +32,10 @@ int line6_init_audio(struct usb_line6 *line6)
> > >  
> > >  	line6->card = card;
> > >  
> > > -	strcpy(card->id, line6->properties->id);
> > > +	strncpy(card->id, line6->properties->id, (sizeof(card->id)-1));
> > >  	strcpy(card->driver, DRIVER_NAME);
> > > -	strcpy(card->shortname, line6->properties->name);
> > > +	strncpy(card->shortname, line6->properties->name,
> > > +			(sizeof(card->shortname)-1));
> > >  	/* longname is 80 chars - see asound.h */
> > >  	sprintf(card->longname, "Line6 %s at USB %s", line6->properties->name,
> > >  		dev_name(line6->ifcdev));
> > 
> > Would not it be better to return -EINVAL (or some other error) instead?
> > 
> > Now you will possibly truncate the name.
> > 
> 
> These don't come from the user, they come from the kernel.
> 
> drivers/staging/line6/driver.c
>     59  
>     60  #define L6PROP(dev_bit, dev_id, dev_name, dev_cap)\
>     61          {.device_bit = LINE6_BIT_##dev_bit, .id = dev_id,\
>     62           .name = dev_name, .capabilities = LINE6_BIT_##dev_cap}
>     63  
>     64  /* *INDENT-OFF* */
>     65  static const struct line6_properties line6_properties_table[] = {
>     66          L6PROP(BASSPODXT,     "BassPODxt",     "BassPODxt",        CTRL_PCM_HW),
>     67          L6PROP(BASSPODXTLIVE, "BassPODxtLive", "BassPODxt Live",   CTRL_PCM_HW),
>     68          L6PROP(BASSPODXTPRO,  "BassPODxtPro",  "BassPODxt Pro",    CTRL_PCM_HW),
>     69          L6PROP(GUITARPORT,    "GuitarPort",    "GuitarPort",       PCM),
>     70          L6PROP(POCKETPOD,     "PocketPOD",     "Pocket POD",       CONTROL),
>     71          L6PROP(PODHD300,      "PODHD300",      "POD HD300",        CTRL_PCM_HW),
>     72          L6PROP(PODHD400,      "PODHD400",      "POD HD400",        CTRL_PCM_HW),
>     73          L6PROP(PODHD500,      "PODHD500",      "POD HD500",        CTRL_PCM_HW),
>     74          L6PROP(PODSTUDIO_GX,  "PODStudioGX",   "POD Studio GX",    PCM),
>     75          L6PROP(PODSTUDIO_UX1, "PODStudioUX1",  "POD Studio UX1",   PCM),
>     76          L6PROP(PODSTUDIO_UX2, "PODStudioUX2",  "POD Studio UX2",   PCM),
>     77          L6PROP(PODX3,         "PODX3",         "POD X3",           PCM),
>     78          L6PROP(PODX3LIVE,     "PODX3Live",     "POD X3 Live",      PCM),
>     79          L6PROP(PODXT,         "PODxt",         "PODxt",            CTRL_PCM_HW),
>     80          L6PROP(PODXTLIVE,     "PODxtLive",     "PODxt Live",       CTRL_PCM_HW),
>     81          L6PROP(PODXTPRO,      "PODxtPro",      "PODxt Pro",        CTRL_PCM_HW),
>     82          L6PROP(TONEPORT_GX,   "TonePortGX",    "TonePort GX",      PCM),
>     83          L6PROP(TONEPORT_UX1,  "TonePortUX1",   "TonePort UX1",     PCM),
>     84          L6PROP(TONEPORT_UX2,  "TonePortUX2",   "TonePort UX2",     PCM),
>     85          L6PROP(VARIAX,        "Variax",        "Variax Workbench", CONTROL),
>     86  };
>     87  /* *INDENT-ON* */
>     88  
> 
> And sadly enough some of those ->id strings are more than 15 characters
> and a NUL which will fit in card->id.  So this overflow is real.  The
> card->shortname is a 32 char array so none of those overflow.
> 
> If we want to sovle the truncation issue then we need to think of
> shorter names for BassPODxtLive, BassPODxtPro, PODStudioUX1, and
> PODStudioUX2.
> 

In that case I suggest compile time assertions that ids and names fit and
a WARN_ON + -EINVAL in line6_init_audio to catch future offenders.

As a side note I'm not sure if pod_try_init from drivers/staging/line6/pod.c
cleans up properly after failed line6_init_audio.

-- 
Mateusz Guzik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/