Re: [PATCH] staging: line6: fix possible overrun

[Mateusz Guzik <mguzik@redhat.com>]

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@gmail.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