linux-kernel - Re: [RFC PATCH 4/6] char: fastrpc: Add support for create remote init process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a0CzU5mGE9YBfe5FVPGCBhjFRsZbgbyy8Yvopu4wQ7uxA@mail.gmail.com>
Date:   Fri, 30 Nov 2018 14:26:23 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        gregkh <gregkh@...uxfoundation.org>,
        Mark Rutland <mark.rutland@....com>,
        DTML <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-arm-msm@...r.kernel.org, bkumar@....qualcomm.com,
        thierry.escande@...aro.org
Subject: Re: [RFC PATCH 4/6] char: fastrpc: Add support for create remote init process

On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
<srinivas.kandagatla@...aro.org> wrote:

> +
> +static int fastrpc_init_process(struct fastrpc_user *fl,
> +                               struct fastrpc_ioctl_init *init)
> +{
> +       struct fastrpc_ioctl_invoke *ioctl;
> +       struct fastrpc_phy_page pages[1];
> +       struct fastrpc_map *file = NULL, *mem = NULL;
> +       struct fastrpc_buf *imem = NULL;
> +       int err = 0;
> +
> +       ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL);
> +       if (!ioctl)
> +               return -ENOMEM;
> +
> +       if (init->flags == FASTRPC_INIT_ATTACH) {
> +               remote_arg_t ra[1];
> +               int tgid = fl->tgid;
> +
> +               ra[0].buf.pv = (void *)&tgid;
> +               ra[0].buf.len = sizeof(tgid);
> +               ioctl->handle = 1;
> +               ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> +               ioctl->pra = ra;
> +               fl->pd = 0;
> +
> +               err = fastrpc_internal_invoke(fl, 1, ioctl);
> +               if (err)
> +                       goto bail;

It doesn't seem right to me to dynamically allocate an 'ioctl' data structure
from kernel context and pass that down to another function. Maybe eliminate
that structure and change fastrpc_internal_invoke to take the individual
arguments here instead of a pointer?

> +       } else if (init->flags == FASTRPC_INIT_CREATE) {

How about splitting each flags== case into a separate function?

> diff --git a/include/uapi/linux/fastrpc.h b/include/uapi/linux/fastrpc.h
> index 8fec66601337..6b596fc7ddf3 100644
> --- a/include/uapi/linux/fastrpc.h
> +++ b/include/uapi/linux/fastrpc.h
> @@ -6,6 +6,12 @@
>  #include <linux/types.h>
>
>  #define FASTRPC_IOCTL_INVOKE   _IOWR('R', 3, struct fastrpc_ioctl_invoke)
> +#define FASTRPC_IOCTL_INIT     _IOWR('R', 4, struct fastrpc_ioctl_init)
> +
> +/* INIT a new process or attach to guestos */
> +#define FASTRPC_INIT_ATTACH      0
> +#define FASTRPC_INIT_CREATE      1
> +#define FASTRPC_INIT_CREATE_STATIC  2

Maybe use three command codes here, and remove the 'flags' member?

> @@ -53,4 +59,16 @@ struct fastrpc_ioctl_invoke {
>         unsigned int *crc;
>  };
>
> +struct fastrpc_ioctl_init {
> +       uint32_t flags;         /* one of FASTRPC_INIT_* macros */
> +       uintptr_t file;         /* pointer to elf file */
> +       uint32_t filelen;       /* elf file length */
> +       int32_t filefd;         /* ION fd for the file */

What does this have to do with ION? The driver seems to otherwise
just use the generic dma_buf interfaces.

> +       uintptr_t mem;          /* mem for the PD */
> +       uint32_t memlen;        /* mem length */
> +       int32_t memfd;          /* fd for the mem */
> +       int attrs;
> +       unsigned int siglen;
> +};

This structure is again not suitable for ioctls. Please used fixed-length
members and no holes in the structure.

       Arnd