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
