Re: [patch] usb: gadget: f_fs: signedness bug in __ffs_func_bind_do_descs()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux