Hi, On Tue, Sep 09, 2014 at 06:37:02PM +0200, Michal Nazarewicz wrote: > On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > On Tue, Sep 09, 2014 at 03:57:26PM +0200, Michal Nazarewicz wrote: > >> On Tue, Sep 09 2014, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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:
signature.asc
Description: Digital signature