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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 27 Apr 2014 00:36:21 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Mateusz Guzik <mguzik@...hat.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 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.

regards,
dan carpenter
--
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/