Re: [PATCH] staging: line6: fix possible overrun
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 > --- > 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. -- Mateusz Guzik ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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
> > > ---
> > > 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: line6: fix possible overrun
On Sun, Apr 27, 2014 at 08:39:32PM +0300, Dan Carpenter wrote: > On Sat, Apr 26, 2014 at 11:59:46PM +0200, Mateusz Guzik wrote: > > > 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 > > That sounds like some magic code which I would love to see. :) > Just asserting something on compile time is not a problem. The kernel has BUILD_BUG_ON macro. I didn't check why, but it doesn't use _Static_assert. Instead it produces some code which makes it unusable in this context. Aforementoined _Static_assert is available at least in gcc and clang and you can call it outside of any function, e.g.: _Static_assert(sizeof(meh) < 42, "oh no"); Unfortnately I failed to come up with a macro which would allow me to use it in the initializer. :/ One could change line6_properties's definition so that it contains arrays instead of pointers, that would introduce automagic checking and I don't think memory waste (if any) would be problematic. > > and a WARN_ON + -EINVAL in line6_init_audio to catch future > > offenders. > > Returning -EINVAL is a bad idea because it would break the driver > completely and make it unusable. > Well I would vote for returning the error anyway. Something is wrong, better fix it as it is instead of risking additional bugs resulting from truncation. > > > > 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. > > Yeah. It doesn't seem to clean up at all. > -- Mateusz Guzik ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: line6: fix possible overrun
On Tue, Apr 29, 2014 at 04:47:11PM +0200, Takashi Iwai wrote: > At Mon, 28 Apr 2014 01:44:25 +0300, > Dan Carpenter wrote: > > > > On Sun, Apr 27, 2014 at 10:00:43PM +0200, Mateusz Guzik wrote: > > > > > and a WARN_ON + -EINVAL in line6_init_audio to catch future > > > > > offenders. > > > > > > > > Returning -EINVAL is a bad idea because it would break the driver > > > > completely and make it unusable. > > > > > > > > > > Well I would vote for returning the error anyway. > > > > I'm trying to be polite, but you are talking about adding regressions > > deliberately... > > > > It's very rare for people to deliberately add regressions to the kernel. > > I have only seen it one time before. > > I don't think Dan would be against returning -EINVAL if all the > offender codes have been fixed (e.g. truncating strings to fit with > the fixed arrays) at first. Then it'd be a good help to catch any > future bugs. But, having -EINVAL without fixing the caller side means > essentially that you're introducing the breakage intentionally > although you know it certainly breaks, which is obviously bad. > > We clearly have a serious miscommunication here (and apparently it started with me not addressing the concern of complete driver breakage). line6_init_audio consumers have to be fixed first, no doubt about that. I was only commenting on catching *future* offenders, which I thought would implictly mean *afterwards*. With that in mind it would seem we are in agreement after all. :-) As far getting this done maybe OP is interested. -- Mateusz Guzik ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723au: fix potential leak in update_bcn_wps_ie()
On Thu, May 01, 2014 at 01:57:27PM +0200, Christian Engelmayer wrote:
> Fix a potential leak in the error path of function update_bcn_wps_ie().
> Make sure that allocated memory for 'pbackup_remainder_ie' is freed
> upon return. Detected by Coverity - CID 1077718.
>
if (remainder_ielen > 0) {
pbackup_remainder_ie = kmalloc(remainder_ielen, GFP_ATOMIC);
if (pbackup_remainder_ie)
memcpy(pbackup_remainder_ie, premainder_ie,
remainder_ielen);
}
pwps_ie_src = pmlmepriv->wps_beacon_ie;
if (pwps_ie_src == NULL)
return;
Maybe just check pwps_ie_src earlier?
--
Mateusz Guzik
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
