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.
This patch reuses the data copied in
diva_xdi_open_adapter() and passes it to diva_xdi_write(). This way, the
above issues can be avoided.
Signed-off-by: Wenwen Wang <[email protected]>
---
drivers/isdn/hardware/eicon/diva.c | 20 +++++++++++++-------
drivers/isdn/hardware/eicon/diva.h | 5 +++--
drivers/isdn/hardware/eicon/divasmain.c | 18 +++++++++++-------
3 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/drivers/isdn/hardware/eicon/diva.c b/drivers/isdn/hardware/eicon/diva.c
index 944a7f3..fa239d8 100644
--- a/drivers/isdn/hardware/eicon/diva.c
+++ b/drivers/isdn/hardware/eicon/diva.c
@@ -388,10 +388,9 @@ void divasa_xdi_driver_unload(void)
** Receive and process command from user mode utility
*/
void *diva_xdi_open_adapter(void *os_handle, const void __user *src,
- int length,
+ int length, diva_xdi_um_cfg_cmd_t *msg,
divas_xdi_copy_from_user_fn_t cp_fn)
{
- diva_xdi_um_cfg_cmd_t msg;
diva_os_xdi_adapter_t *a = NULL;
diva_os_spin_lock_magic_t old_irql;
struct list_head *tmp;
@@ -401,21 +400,21 @@ void *diva_xdi_open_adapter(void *os_handle, const void __user *src,
length, sizeof(diva_xdi_um_cfg_cmd_t)))
return NULL;
}
- if ((*cp_fn) (os_handle, &msg, src, sizeof(msg)) <= 0) {
+ if ((*cp_fn) (os_handle, msg, src, sizeof(*msg)) <= 0) {
DBG_ERR(("A: A(?) open, write error"))
return NULL;
}
diva_os_enter_spin_lock(&adapter_lock, &old_irql, "open_adapter");
list_for_each(tmp, &adapter_queue) {
a = list_entry(tmp, diva_os_xdi_adapter_t, link);
- if (a->controller == (int)msg.adapter)
+ if (a->controller == (int)msg->adapter)
break;
a = NULL;
}
diva_os_leave_spin_lock(&adapter_lock, &old_irql, "open_adapter");
if (!a) {
- DBG_ERR(("A: A(%d) open, adapter not found", msg.adapter))
+ DBG_ERR(("A: A(%d) open, adapter not found", msg->adapter))
}
return (a);
@@ -437,7 +436,8 @@ void diva_xdi_close_adapter(void *adapter, void *os_handle)
int
diva_xdi_write(void *adapter, void *os_handle, const void __user *src,
- int length, divas_xdi_copy_from_user_fn_t cp_fn)
+ int length, diva_xdi_um_cfg_cmd_t *msg,
+ divas_xdi_copy_from_user_fn_t cp_fn)
{
diva_os_xdi_adapter_t *a = (diva_os_xdi_adapter_t *) adapter;
void *data;
@@ -459,7 +459,13 @@ diva_xdi_write(void *adapter, void *os_handle, const void __user *src,
return (-2);
}
- length = (*cp_fn) (os_handle, data, src, length);
+ if (msg) {
+ *(diva_xdi_um_cfg_cmd_t *)data = *msg;
+ length = (*cp_fn) (os_handle, (char *)data + sizeof(*msg),
+ src + sizeof(*msg), length - sizeof(*msg));
+ } else {
+ length = (*cp_fn) (os_handle, data, src, length);
+ }
if (length > 0) {
if ((*(a->interface.cmd_proc))
(a, (diva_xdi_um_cfg_cmd_t *) data, length)) {
diff --git a/drivers/isdn/hardware/eicon/diva.h b/drivers/isdn/hardware/eicon/diva.h
index b067032..eb454c5 100644
--- a/drivers/isdn/hardware/eicon/diva.h
+++ b/drivers/isdn/hardware/eicon/diva.h
@@ -20,10 +20,11 @@ int diva_xdi_read(void *adapter, void *os_handle, void __user *dst,
int max_length, divas_xdi_copy_to_user_fn_t cp_fn);
int diva_xdi_write(void *adapter, void *os_handle, const void __user *src,
- int length, divas_xdi_copy_from_user_fn_t cp_fn);
+ int length, diva_xdi_um_cfg_cmd_t *msg,
+ divas_xdi_copy_from_user_fn_t cp_fn);
void *diva_xdi_open_adapter(void *os_handle, const void __user *src,
- int length,
+ int length, diva_xdi_um_cfg_cmd_t *msg,
divas_xdi_copy_from_user_fn_t cp_fn);
void diva_xdi_close_adapter(void *adapter, void *os_handle);
diff --git a/drivers/isdn/hardware/eicon/divasmain.c b/drivers/isdn/hardware/eicon/divasmain.c
index b9980e8..b6a3950 100644
--- a/drivers/isdn/hardware/eicon/divasmain.c
+++ b/drivers/isdn/hardware/eicon/divasmain.c
@@ -591,19 +591,22 @@ static int divas_release(struct inode *inode, struct file *file)
static ssize_t divas_write(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
+ diva_xdi_um_cfg_cmd_t msg;
int ret = -EINVAL;
if (!file->private_data) {
file->private_data = diva_xdi_open_adapter(file, buf,
- count,
+ count, &msg,
xdi_copy_from_user);
- }
- if (!file->private_data) {
- return (-ENODEV);
+ if (!file->private_data)
+ return (-ENODEV);
+ ret = diva_xdi_write(file->private_data, file,
+ buf, count, &msg, xdi_copy_from_user);
+ } else {
+ ret = diva_xdi_write(file->private_data, file,
+ buf, count, NULL, xdi_copy_from_user);
}
- ret = diva_xdi_write(file->private_data, file,
- buf, count, xdi_copy_from_user);
switch (ret) {
case -1: /* Message should be removed from rx mailbox first */
ret = -EBUSY;
@@ -622,11 +625,12 @@ static ssize_t divas_write(struct file *file, const char __user *buf,
static ssize_t divas_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
+ diva_xdi_um_cfg_cmd_t msg;
int ret = -EINVAL;
if (!file->private_data) {
file->private_data = diva_xdi_open_adapter(file, buf,
- count,
+ count, &msg,
xdi_copy_from_user);
}
if (!file->private_data) {
--
2.7.4
From: Wenwen Wang <[email protected]>
Date: Fri, 18 May 2018 16:33:47 -0500
> 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.
This doesn't even compile:
In file included from drivers/isdn/hardware/eicon/divasmain.c:30:
drivers/isdn/hardware/eicon/diva.h:23:18: error: unknown type name ?diva_xdi_um_cfg_cmd_t?
int length, diva_xdi_um_cfg_cmd_t *msg,
^~~~~~~~~~~~~~~~~~~~~
drivers/isdn/hardware/eicon/diva.h:27:20: error: unknown type name ?diva_xdi_um_cfg_cmd_t?
int length, diva_xdi_um_cfg_cmd_t *msg,
^~~~~~~~~~~~~~~~~~~~~
Hi Wenwen,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.17-rc5 next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Wenwen-Wang/isdn-eicon-fix-a-missing-check-bug/20180521-034229
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from drivers/isdn/hardware/eicon/divasfunc.c:18:0:
>> drivers/isdn/hardware/eicon/diva.h:23:18: error: unknown type name 'diva_xdi_um_cfg_cmd_t'; did you mean 'diva_xdi_capi_cfg_t'?
int length, diva_xdi_um_cfg_cmd_t *msg,
^~~~~~~~~~~~~~~~~~~~~
diva_xdi_capi_cfg_t
drivers/isdn/hardware/eicon/diva.h:27:20: error: unknown type name 'diva_xdi_um_cfg_cmd_t'; did you mean 'diva_xdi_capi_cfg_t'?
int length, diva_xdi_um_cfg_cmd_t *msg,
^~~~~~~~~~~~~~~~~~~~~
diva_xdi_capi_cfg_t
--
In file included from drivers/isdn/hardware/eicon/divasmain.c:30:0:
>> drivers/isdn/hardware/eicon/diva.h:23:18: error: unknown type name 'diva_xdi_um_cfg_cmd_t'
int length, diva_xdi_um_cfg_cmd_t *msg,
^~~~~~~~~~~~~~~~~~~~~
drivers/isdn/hardware/eicon/diva.h:27:20: error: unknown type name 'diva_xdi_um_cfg_cmd_t'
int length, diva_xdi_um_cfg_cmd_t *msg,
^~~~~~~~~~~~~~~~~~~~~
drivers/isdn/hardware/eicon/divasmain.c: In function 'divas_write':
>> drivers/isdn/hardware/eicon/divasmain.c:598:24: error: implicit declaration of function 'diva_xdi_open_adapter'; did you mean 'diva_xdi_close_adapter'? [-Werror=implicit-function-declaration]
file->private_data = diva_xdi_open_adapter(file, buf,
^~~~~~~~~~~~~~~~~~~~~
diva_xdi_close_adapter
>> drivers/isdn/hardware/eicon/divasmain.c:598:22: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
file->private_data = diva_xdi_open_adapter(file, buf,
^
>> drivers/isdn/hardware/eicon/divasmain.c:603:9: error: implicit declaration of function 'diva_xdi_write'; did you mean 'divas_write'? [-Werror=implicit-function-declaration]
ret = diva_xdi_write(file->private_data, file,
^~~~~~~~~~~~~~
divas_write
drivers/isdn/hardware/eicon/divasmain.c: In function 'divas_read':
drivers/isdn/hardware/eicon/divasmain.c:632:22: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
file->private_data = diva_xdi_open_adapter(file, buf,
^
cc1: some warnings being treated as errors
vim +23 drivers/isdn/hardware/eicon/diva.h
12
13 typedef int (*divas_xdi_copy_to_user_fn_t) (void *os_handle, void __user *dst,
14 const void *src, int length);
15
16 typedef int (*divas_xdi_copy_from_user_fn_t) (void *os_handle, void *dst,
17 const void __user *src, int length);
18
19 int diva_xdi_read(void *adapter, void *os_handle, void __user *dst,
20 int max_length, divas_xdi_copy_to_user_fn_t cp_fn);
21
22 int diva_xdi_write(void *adapter, void *os_handle, const void __user *src,
> 23 int length, diva_xdi_um_cfg_cmd_t *msg,
24 divas_xdi_copy_from_user_fn_t cp_fn);
25
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sun, May 20, 2018 at 5:37 PM, David Miller <[email protected]> wrote:
> From: Wenwen Wang <[email protected]>
> Date: Fri, 18 May 2018 16:33:47 -0500
>
>> 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.
>
> This doesn't even compile:
>
> In file included from drivers/isdn/hardware/eicon/divasmain.c:30:
> drivers/isdn/hardware/eicon/diva.h:23:18: error: unknown type name ‘diva_xdi_um_cfg_cmd_t’
> int length, diva_xdi_um_cfg_cmd_t *msg,
> ^~~~~~~~~~~~~~~~~~~~~
> drivers/isdn/hardware/eicon/diva.h:27:20: error: unknown type name ‘diva_xdi_um_cfg_cmd_t’
> int length, diva_xdi_um_cfg_cmd_t *msg,
> ^~~~~~~~~~~~~~~~~~~~~
Sorry. I will correct the compilation errors and resubmit the patch.
Wenwen