Re: [PATCH v4 3/5] misc: fastrpc: Add support for context Invoke method

[Srinivas Kandagatla]

Thanks for the review,

I will fix them and send new version!

On 31/01/2019 15:34, Greg KH wrote:
> On Thu, Jan 24, 2019 at 03:24:10PM +0000, Srinivas Kandagatla wrote:
> > This patch adds support to compute context invoke method
> > on the remote processor (DSP).
> > This involves setting up the functions input and output arguments,
> > input and output handles and mapping the dmabuf fd for the
> > argument/handle buffers.
>
> This says _what_ this code does, but not why.  What about all of that
> explaination you had in the 0/5 patch, shouldn't that be here, or on
> patch 2/5?
Yes, I will add more details in to the log.

> Some nits below:
>
> > +static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
> > +{
> > +	struct fastrpc_invoke_args *args = NULL;
> > +	struct fastrpc_invoke inv;
> > +	u32 nscalars;
> > +	int err;
> > +
> > +	if (copy_from_user(&inv, argp, sizeof(inv)))
> > +		return -EFAULT;
> > +
> > +	nscalars = REMOTE_SCALARS_LENGTH(inv.sc);
> > +	if (nscalars) {
> > +		args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
>
> Yeah, let's not bounds check the input variables and suck up all of the
> kernel memory!
>
> Remember:
> 	ALL INPUT IS EVIL

I will add more checks here and other such instances in next version....
> > +static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
> > +				 unsigned long arg)
> > +{
> > +	struct fastrpc_user *fl = (struct fastrpc_user *)file->private_data;
> > +	char __user *argp = (char __user *)arg;
> > +	int err;
> > +
> > +	switch (cmd) {
> > +	case FASTRPC_IOCTL_INVOKE:
> > +		err = fastrpc_invoke(fl, argp);
> > +		break;
> > +	default:
> > +		err = -ENOTTY;
> > +		dev_err(fl->sctx->dev, "bad ioctl: %d\n", cmd);
>
> Don't spam the syslog if someone sends you an invalid ioctl.  That's a
> sure way to DoS the system.
will fix this in next version.
> > +		break;
> > +	}
> > +
> > +	if (err)
> > +		dev_dbg(fl->sctx->dev, "Error: IOCTL Failed with %d\n", err);
> > +
> > +	return err;
> > +}
> > +
> >   static const struct file_operations fastrpc_fops = {
> >   	.open = fastrpc_device_open,
> >   	.release = fastrpc_device_release,
> > +	.unlocked_ioctl = fastrpc_device_ioctl,
> > +	.compat_ioctl = fastrpc_device_ioctl,
> >   };
> >   
> >   static int fastrpc_cb_probe(struct platform_device *pdev)
> > @@ -260,9 +932,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> >   	return of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> >   }
> >   
> > +static void fastrpc_notify_users(struct fastrpc_user *user)
> > +{
> > +	struct fastrpc_invoke_ctx *ctx, *n;
> > +
> > +	spin_lock(&user->lock);
> > +	list_for_each_entry_safe(ctx, n, &user->pending, node)
> > +		complete(&ctx->work);
>
> Why safe?  You aren't deleting the list here.
Not sure why it ended up with safe here, does not make sense unless am 
deleting it.. will fix this in next version.

...>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> > new file mode 100644
> > index 000000000000..a69ef33dc37e
> > --- /dev/null
> > +++ b/include/uapi/misc/fastrpc.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef __QCOM_FASTRPC_H__
> > +#define __QCOM_FASTRPC_H__
> > +
> > +#include <linux/types.h>
> > +
> > +#define FASTRPC_IOCTL_INVOKE		_IOWR('R', 3, struct fastrpc_invoke)
> > +
> > +struct fastrpc_invoke_args {
> > +	__u64 ptr;
> > +	__u64 length;
> > +	__s32 fd;
> > +	__u32 reserved;
>
> Are you checking that reserved is all 0 now?

No, I should add the checks!

> > +};
> > +
> > +struct fastrpc_invoke {
> > +	__u32 handle;
> > +	__u32 sc;
> > +	__u64 args;
> > +};
>
> Do you need packed here?  What about endian issues?
We do not need this packed here, as this is not the actual structure 
that the passed to the DSP.

Thanks,
srini

> thanks,
>
> greg k-h