* Re: [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs()
2014-09-09 12:06 [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs() Dan Carpenter
@ 2014-09-09 13:57 ` Michal Nazarewicz
2014-09-09 14:25 ` Dan Carpenter
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Michal Nazarewicz @ 2014-09-09 13:57 UTC (permalink / raw)
To: kernel-janitors
On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We need "idx" to be signed for the error handling to work.
>
> Fixes: 6d5c1c77bbf9 ('usb: gadget: f_fs: fix the redundant ep files problem')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> Btw, there is a sparse warning:
>
> drivers/usb/gadget/function/f_fs.c:401:44: warning: Variable length array is used.
>
> The risk here is that the array would be too large. I don't know the
> code well enough to say if it can be triggered, but from an outsider
> perspective it looks scary (security implications). There should be a
> comment explaining why it can't be used to overflow the 8k stack.
n in that function can be at most 4 and usb_functionfs_event is 20 bytes
long so this takes at most 80 bytes. Having said that, I can prepare
a patch that converts the array to one with compile-time size if
desired.
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 0dc3552..7ad7137 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2393,7 +2393,8 @@ static int __ffs_func_bind_do_descs(enum ffs_entity_type type, u8 *valuep,
> struct usb_endpoint_descriptor *ds = (void *)desc;
> struct ffs_function *func = priv;
> struct ffs_ep *ffs_ep;
> - unsigned ep_desc_id, idx;
> + unsigned ep_desc_id;
> + int idx;
> static const char *speed_names[] = { "full", "high", "super" };
>
> if (type != FFS_DESCRIPTOR)
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs()
2014-09-09 12:06 [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs() Dan Carpenter
2014-09-09 13:57 ` Michal Nazarewicz
@ 2014-09-09 14:25 ` Dan Carpenter
2014-09-09 16:37 ` Michal Nazarewicz
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2014-09-09 14:25 UTC (permalink / raw)
To: kernel-janitors
On Tue, Sep 09, 2014 at 03:57:26PM +0200, Michal Nazarewicz wrote:
> On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Btw, there is a sparse warning:
> >
> > drivers/usb/gadget/function/f_fs.c:401:44: warning: Variable length array is used.
> >
> > The risk here is that the array would be too large. I don't know the
> > code well enough to say if it can be triggered, but from an outsider
> > perspective it looks scary (security implications). There should be a
> > comment explaining why it can't be used to overflow the 8k stack.
>
> n in that function can be at most 4
I looked for where this limit is set but couldn't figure it out. Which
function is it?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs()
2014-09-09 12:06 [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs() Dan Carpenter
2014-09-09 13:57 ` Michal Nazarewicz
2014-09-09 14:25 ` Dan Carpenter
@ 2014-09-09 16:37 ` Michal Nazarewicz
2014-09-09 16:40 ` Felipe Balbi
2014-09-10 11:05 ` Dan Carpenter
4 siblings, 0 replies; 6+ messages in thread
From: Michal Nazarewicz @ 2014-09-09 16:37 UTC (permalink / raw)
To: kernel-janitors
On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Sep 09, 2014 at 03:57:26PM +0200, Michal Nazarewicz wrote:
>> On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > Btw, there is a sparse warning:
>> >
>> > drivers/usb/gadget/function/f_fs.c:401:44: warning: Variable length array is used.
>> >
>> > The risk here is that the array would be too large. I don't know the
>> > code well enough to say if it can be triggered, but from an outsider
>> > perspective it looks scary (security implications). There should be a
>> > comment explaining why it can't be used to overflow the 8k stack.
>>
>> n in that function can be at most 4
>
> I looked for where this limit is set but couldn't figure it out. Which
> function is it?
The limit is never explicitly set, but logic in this function guarantees
it:
static void __ffs_event_add(struct ffs_data *ffs,
enum usb_functionfs_event_type type)
{
enum usb_functionfs_event_type rem_type1, rem_type2 = type;
int neg = 0;
/*
* Abort any unhandled setup
*
* We do not need to worry about some cmpxchg() changing value
* of ffs->setup_state without holding the lock because when
* state is FFS_SETUP_PENDING cmpxchg() in several places in
* the source does nothing.
*/
if (ffs->setup_state = FFS_SETUP_PENDING)
ffs->setup_state = FFS_SETUP_CANCELLED;
switch (type) {
case FUNCTIONFS_RESUME:
rem_type2 = FUNCTIONFS_SUSPEND;
/* FALL THROUGH */
case FUNCTIONFS_SUSPEND:
case FUNCTIONFS_SETUP:
rem_type1 = type;
/* Discard all similar events */
break;
case FUNCTIONFS_BIND:
case FUNCTIONFS_UNBIND:
case FUNCTIONFS_DISABLE:
case FUNCTIONFS_ENABLE:
/* Discard everything other then power management. */
rem_type1 = FUNCTIONFS_SUSPEND;
rem_type2 = FUNCTIONFS_RESUME;
neg = 1;
break;
default:
BUG();
}
{
u8 *ev = ffs->ev.types, *out = ev;
unsigned n = ffs->ev.count;
for (; n; --n, ++ev)
if ((*ev = rem_type1 || *ev = rem_type2) = neg)
*out++ = *ev;
else
pr_vdebug("purging event %d\n", *ev);
ffs->ev.count = out - ffs->ev.types;
}
pr_vdebug("adding event %d\n", type);
ffs->ev.types[ffs->ev.count++] = type;
wake_up_locked(&ffs->ev.waitq);
}
Looking at the last four cases, BIND, UNBIND, DISABLE and ENABLE events
will never be present on the event list at the same time. Since there's
only three more event types, this means that the list can contain at
most four events.
So ffs->ev.count <= 4, and since __ffs_ep0_read_events is called with
n = min(n, (size_t)ffs->ev.count)), n <= 4.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs()
2014-09-09 12:06 [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs() Dan Carpenter
` (2 preceding siblings ...)
2014-09-09 16:37 ` Michal Nazarewicz
@ 2014-09-09 16:40 ` Felipe Balbi
2014-09-10 11:05 ` Dan Carpenter
4 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2014-09-09 16:40 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]
Hi,
On Tue, Sep 09, 2014 at 06:37:02PM +0200, Michal Nazarewicz wrote:
> On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Sep 09, 2014 at 03:57:26PM +0200, Michal Nazarewicz wrote:
> >> On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > Btw, there is a sparse warning:
> >> >
> >> > drivers/usb/gadget/function/f_fs.c:401:44: warning: Variable length array is used.
> >> >
> >> > The risk here is that the array would be too large. I don't know the
> >> > code well enough to say if it can be triggered, but from an outsider
> >> > perspective it looks scary (security implications). There should be a
> >> > comment explaining why it can't be used to overflow the 8k stack.
> >>
> >> n in that function can be at most 4
> >
> > I looked for where this limit is set but couldn't figure it out. Which
> > function is it?
>
> The limit is never explicitly set, but logic in this function guarantees
> it:
>
> static void __ffs_event_add(struct ffs_data *ffs,
> enum usb_functionfs_event_type type)
> {
> enum usb_functionfs_event_type rem_type1, rem_type2 = type;
> int neg = 0;
>
> /*
> * Abort any unhandled setup
> *
> * We do not need to worry about some cmpxchg() changing value
> * of ffs->setup_state without holding the lock because when
> * state is FFS_SETUP_PENDING cmpxchg() in several places in
> * the source does nothing.
> */
> if (ffs->setup_state == FFS_SETUP_PENDING)
> ffs->setup_state = FFS_SETUP_CANCELLED;
>
> switch (type) {
> case FUNCTIONFS_RESUME:
> rem_type2 = FUNCTIONFS_SUSPEND;
> /* FALL THROUGH */
> case FUNCTIONFS_SUSPEND:
> case FUNCTIONFS_SETUP:
> rem_type1 = type;
> /* Discard all similar events */
> break;
>
> case FUNCTIONFS_BIND:
> case FUNCTIONFS_UNBIND:
> case FUNCTIONFS_DISABLE:
> case FUNCTIONFS_ENABLE:
> /* Discard everything other then power management. */
> rem_type1 = FUNCTIONFS_SUSPEND;
> rem_type2 = FUNCTIONFS_RESUME;
> neg = 1;
> break;
>
> default:
> BUG();
[off topic]
not sure a BUG() is the right way to go here. I'd rather see a WARN()
instead, with early return. We really don't want to crash the entire
system because someone passed an invalid type as argument.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs()
2014-09-09 12:06 [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs() Dan Carpenter
` (3 preceding siblings ...)
2014-09-09 16:40 ` Felipe Balbi
@ 2014-09-10 11:05 ` Dan Carpenter
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2014-09-10 11:05 UTC (permalink / raw)
To: kernel-janitors
On Tue, Sep 09, 2014 at 06:37:02PM +0200, Michal Nazarewicz wrote:
> On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Sep 09, 2014 at 03:57:26PM +0200, Michal Nazarewicz wrote:
> >> On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > Btw, there is a sparse warning:
> >> >
> >> > drivers/usb/gadget/function/f_fs.c:401:44: warning: Variable length array is used.
> >> >
> >> > The risk here is that the array would be too large. I don't know the
> >> > code well enough to say if it can be triggered, but from an outsider
> >> > perspective it looks scary (security implications). There should be a
> >> > comment explaining why it can't be used to overflow the 8k stack.
> >>
> >> n in that function can be at most 4
> >
> > I looked for where this limit is set but couldn't figure it out. Which
> > function is it?
>
> The limit is never explicitly set, but logic in this function guarantees
> it:
>
Ok. Thanks. I maybe could have found this on my own because I store
this sort of information in Smatch except that "ev" is an anonymous
struct.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread