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

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

[Wenwen Wang]

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@umn.edu>
---
 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

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

[YU Bo]

Hello,
I am just notice your subject line.There are missing something i think
On Sat, May 05, 2018 at 02:32:46PM -0500, Wenwen Wang 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@umn.edu>
>---
> 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
>

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@umn.edu> 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@umn.edu>
> ---
>  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
>

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

[Tobin C. Harding]

On Wed, May 09, 2018 at 12:30:18AM -0500, Wenwen Wang wrote:
> Hello
> 
> Could you please review this patch? We need a confirmation because we
> are working on an approaching deadline.

I didn't know 'we' had deadlines :)


	Tobin

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

[David Miller]

From: Wenwen Wang <wang6495@umn.edu>
Date: Sat,  5 May 2018 14:32:46 -0500

> 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.

Better fix is to copy the msg header once into an on-stack buffer supplied
by diva_write() to diva_xdi_open_adapter(), which is then passed on to
diva_xdi_write() with an adjusted src pointer and length.

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

[Wenwen Wang]

Thanks for your suggestion, David! I will revise the patch and resubmit it.

Wenwen

On Fri, May 11, 2018 at 2:50 PM, David Miller <davem@davemloft.net> wrote:
> From: Wenwen Wang <wang6495@umn.edu>
> Date: Sat,  5 May 2018 14:32:46 -0500
>
>> 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.
>
> Better fix is to copy the msg header once into an on-stack buffer supplied
> by diva_write() to diva_xdi_open_adapter(), which is then passed on to
> diva_xdi_write() with an adjusted src pointer and length.