usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs()
Subsystems (2)
PATCH 1/1 usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs()
We need "idx" to be signed for the error handling to work.
---
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.
drivers/usb/gadget/function/f_fs.c +2 -1
@@ -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 )
Quoted 5 lines · Dan Carpenter
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 <[email protected]>
Quoted 9 lines
---
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.
Quoted 15 lines
drivers/usb/gadget/function/f_fs.c +2
-1@@ -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 )
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
Quoted 11 lines · Michal Nazarewicz
Quoted 8 lines · Dan Carpenter
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
Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Michal Nazarewicz
Quoted 8 lines · Dan Carpenter
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.
Quoted 4 lines
Quoted 1 line
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.
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
Hi,
Quoted 51 lines · Michal Nazarewicz
Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Michal Nazarewicz
Quoted 8 lines · Dan Carpenter
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.
Quoted 4 lines
Quoted 1 line
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,
enumusb_functionfs_event_typetype){ 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:
Quoted 8 lines
/* 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.
Signature · 1 line
balbi
Quoted 12 lines · Michal Nazarewicz
Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Michal Nazarewicz
Quoted 8 lines · Dan Carpenter
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.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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