[PATCH 4/9] VC04_SERVICES: Add compat ioctl handler for "queue message"

[Michael Zoran]

On Thu, 2017-01-19 at 12:14 +0300, Dan Carpenter wrote:
> On Wed, Jan 18, 2017 at 07:04:48AM -0800, Michael Zoran wrote:
> > Add compat handler for "queue message" ioctl.
> > 
> > Signed-off-by: Michael Zoran <mzoran at crowfest.net>
> > ---
> >  .../vc04_services/interface/vchiq_arm/vchiq_arm.c  | 36
> > ++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git
> > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index e26949247f91..1c0afa318036 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -1271,6 +1271,41 @@ vchiq_ioctl_compat_internal(
> >  		}
> >  	} break;
> >  
> > +
> > +		for (i = 0; i < args32.count; i++) {
> > +			elements[i].data =
> > compat_ptr(elements32[i].data);
> > +			elements[i].size = elements32[i].size;
> > +		}
> > +
> > +		status = vchiq_ioc_queue_message(args32.handle,
> > +						 elements,
> > args32.count);
> 
> Btw, I notice that in vchiq_ioc_queue_message() "total_size +=
> elements[i].size;" can have an integer overflow.  What are the
> security
> implications of that?

I don't think it has much security implications. I did some checking
and I think all an integer overflow would do is truncate the message.
Meaning both the coping and the memory allocation code will both use
the truncated integer. That should be the same as if the application
just legitimately sent a truncated message.

In general, I think the firmware needs to gracefully handle garbage
messages as this driver can't prevent that and it really shouldn't be
into the business of validating garbage messages.

I do think in the case of an integer overflow, it's better to return an
error and possibly log an error message to the kernel log for easier
debugging then just let it go.