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 <[email protected]>
---
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));
--
1.9.1
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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
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. :)
> 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.
>
> 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.
regards,
dan carpenter
Thank's for your answers,
will try to look deeper,
>> >
>> > 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. :)
>
>> 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.
>
>>
>> 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.
>
Laurent Navet.
--
« On ne résout pas un problème avec les modes de pensée qui l’ont engendré. »
« You cannot solve current problems with current thinking. Current
problems are the result of current thinking »
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
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.
regards,
dan carpenter
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.
Takashi
Yeah. If this were a brand new driver then returning -EINVAL would be a
good idea.
Smatch actually warns about this code as well if you turn on the
--spammy option. But there are too many of these kinds of warnings and
even I can't check them all so the warning is basically useless.
In a few months I will have improved the Smatch code to know that the
source string is too large so this bug could have been avoided.
regards,
dan carpenter
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
We have two options:
1) Do we risk introducing new truncation bugs.
2) Do we risk breaking the driver because we didn't catch every
truncation bug.
Truncation bugs here are very low impact and probably no one would even
notice. That's how not worried I am about truncation bugs in this
context. It's also unlikely that we will introduce any new truncation
bugs.
Breaking the driver is very serious. But on the other hand as soon as
you proposed introducing an -EINVAL, I reviewed the driver and found the
places which would break when the code was changed. As a result of my
review then the likely hood of breakage is now low.
In other words both options are probably fine. If someone is able to
test the -EINVAL change then I would be happy to have it.
regards,
dan carpenter