Re: [PATCH] isdn: eicon: fix a missing-check bug

[Wenwen Wang]

Hello

Could you please review this patch? We need a confirmation because we
are working on an approaching deadline.

Thanks!
Wenwen

On Sat, May 5, 2018 at 2:32 PM, Wenwen Wang <wang6495@xxxxxxx> wrote:
> In divasmain.c, the function divas_write() firstly invokes the function
> diva_xdi_open_adapter() to open the adapter that matches with the adapter
> number provided by the user, and then invokes the function diva_xdi_write()
> to perform the write operation using the matched adapter. The two functions
> diva_xdi_open_adapter() and diva_xdi_write() are located in diva.c.
>
> In diva_xdi_open_adapter(), the user command is copied to the object 'msg'
> from the userspace pointer 'src' through the function pointer 'cp_fn',
> which eventually calls copy_from_user() to do the copy. Then, the adapter
> number 'msg.adapter' is used to find out a matched adapter from the
> 'adapter_queue'. A matched adapter will be returned if it is found.
> Otherwise, NULL is returned to indicate the failure of the verification on
> the adapter number.
>
> As mentioned above, if a matched adapter is returned, the function
> diva_xdi_write() is invoked to perform the write operation. In this
> function, the user command is copied once again from the userspace pointer
> 'src', which is the same as the 'src' pointer in diva_xdi_open_adapter() as
> both of them are from the 'buf' pointer in divas_write(). Similarly, the
> copy is achieved through the function pointer 'cp_fn', which finally calls
> copy_from_user(). After the successful copy, the corresponding command
> processing handler of the matched adapter is invoked to perform the write
> operation.
>
> It is obvious that there are two copies here from userspace, one is in
> diva_xdi_open_adapter(), and one is in diva_xdi_write(). Plus, both of
> these two copies share the same source userspace pointer, i.e., the 'buf'
> pointer in divas_write(). Given that a malicious userspace process can race
> to change the content pointed by the 'buf' pointer, this can pose potential
> security issues. For example, in the first copy, the user provides a valid
> adapter number to pass the verification process and a valid adapter can be
> found. Then the user can modify the adapter number to an invalid number.
> This way, the user can bypass the verification process of the adapter
> number and inject inconsistent data.
>
> To avoid such issues, this patch adds a check after the second copy in the
> function diva_xdi_write(). If the adapter number is not equal to the one
> obtained in the first copy, (-4) will be returned to divas_write(), which
> will then return an error code -EINVAL.
>
> Signed-off-by: Wenwen Wang <wang6495@xxxxxxx>
> ---
>  drivers/isdn/hardware/eicon/diva.c      | 6 +++++-
>  drivers/isdn/hardware/eicon/divasmain.c | 3 +++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/isdn/hardware/eicon/diva.c b/drivers/isdn/hardware/eicon/diva.c
> index 944a7f3..46cbf76 100644
> --- a/drivers/isdn/hardware/eicon/diva.c
> +++ b/drivers/isdn/hardware/eicon/diva.c
> @@ -440,6 +440,7 @@ diva_xdi_write(void *adapter, void *os_handle, const void __user *src,
>                int length, divas_xdi_copy_from_user_fn_t cp_fn)
>  {
>         diva_os_xdi_adapter_t *a = (diva_os_xdi_adapter_t *) adapter;
> +       diva_xdi_um_cfg_cmd_t *p;
>         void *data;
>
>         if (a->xdi_mbox.status & DIVA_XDI_MBOX_BUSY) {
> @@ -461,7 +462,10 @@ diva_xdi_write(void *adapter, void *os_handle, const void __user *src,
>
>         length = (*cp_fn) (os_handle, data, src, length);
>         if (length > 0) {
> -               if ((*(a->interface.cmd_proc))
> +               p = (diva_xdi_um_cfg_cmd_t *) data;
> +               if (a->controller != (int)p->adapter) {
> +                       length = -4;
> +               } else if ((*(a->interface.cmd_proc))
>                     (a, (diva_xdi_um_cfg_cmd_t *) data, length)) {
>                         length = -3;
>                 }
> diff --git a/drivers/isdn/hardware/eicon/divasmain.c b/drivers/isdn/hardware/eicon/divasmain.c
> index b9980e8..a03c658 100644
> --- a/drivers/isdn/hardware/eicon/divasmain.c
> +++ b/drivers/isdn/hardware/eicon/divasmain.c
> @@ -614,6 +614,9 @@ static ssize_t divas_write(struct file *file, const char __user *buf,
>         case -3:
>                 ret = -ENXIO;
>                 break;
> +       case -4:
> +               ret = -EINVAL;
> +               break;
>         }
>         DBG_TRC(("write: ret %d", ret));
>         return (ret);
> --
> 2.7.4
>