Bug 188451 - Function lbs_cmd_802_11_sleep_params() returns an improper error code when the call to lbs_cmd_with_response() fails, which may result in stack information leak
Summary: Function lbs_cmd_802_11_sleep_params() returns an improper error code when th...
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: network-wireless (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: drivers_network-wireless@kernel-bugs.osdl.org
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-24 01:24 UTC by RUC_Soft_Sec
Modified: 2017-05-11 09:56 UTC (History)
1 user (show)

See Also:
Kernel Version: linux-4.9-rc6
Subsystem:
Regression: No
Bisected commit-id:


Attachments
The patch fixes the bug (1.37 KB, patch)
2017-05-11 09:51 UTC, bianpan
Details | Diff

Description RUC_Soft_Sec 2016-11-24 01:24:43 UTC
Function lbs_cmd_802_11_sleep_params() defined in file drivers/net/wireless/marvell/libertas/cmd.c returns 0 (indicates success) even when the call to  bs_cmd_with_response() (at line 290) fails. When something goes wrong with bs_cmd_with_response(), the parameter sp of function lbs_cmd_802_11_sleep_params() will keep uninitialized. In this case, its caller, function lbs_sleepparams_read() defined in file drivers/net/wireless/marvell/libertas/debugfs.c, will copy uninitialized stack memory to "userbuf", which may result in information leak. I think use "return ret;" instead of "return 0;" in function lbs_cmd_802_11_sleep_params() can avoid the above problem. Codes related to this bug are shown as below.

lbs_cmd_802_11_sleep_params @@ drivers/net/wireless/marvell/libertas/cmd.c
 269 int lbs_cmd_802_11_sleep_params(struct lbs_private *priv, uint16_t cmd_action,
 270                 struct sleep_params *sp)
 271 {
 272     struct cmd_ds_802_11_sleep_params cmd;
 273     int ret;
 274 
 275     lbs_deb_enter(LBS_DEB_CMD);
 276 
 277     if (cmd_action == CMD_ACT_GET) {
 278         memset(&cmd, 0, sizeof(cmd));
 279     } else {
 280         cmd.error = cpu_to_le16(sp->sp_error);
 281         cmd.offset = cpu_to_le16(sp->sp_offset);
 282         cmd.stabletime = cpu_to_le16(sp->sp_stabletime);
 283         cmd.calcontrol = sp->sp_calcontrol;
 284         cmd.externalsleepclk = sp->sp_extsleepclk;
 285         cmd.reserved = cpu_to_le16(sp->sp_reserved);
 286     }
 287     cmd.hdr.size = cpu_to_le16(sizeof(cmd));
 288     cmd.action = cpu_to_le16(cmd_action);
 289 
 290     ret = lbs_cmd_with_response(priv, CMD_802_11_SLEEP_PARAMS, &cmd);
 291 
 292     if (!ret) {
 293         lbs_deb_cmd("error 0x%x, offset 0x%x, stabletime 0x%x, "
 294                 "calcontrol 0x%x extsleepclk 0x%x\n",
 295                 le16_to_cpu(cmd.error), le16_to_cpu(cmd.offset),
 296                 le16_to_cpu(cmd.stabletime), cmd.calcontrol,
 297                 cmd.externalsleepclk);
 298 
 299         sp->sp_error = le16_to_cpu(cmd.error);
 300         sp->sp_offset = le16_to_cpu(cmd.offset);
 301         sp->sp_stabletime = le16_to_cpu(cmd.stabletime);
 302         sp->sp_calcontrol = cmd.calcontrol;
 303         sp->sp_extsleepclk = cmd.externalsleepclk;
 304         sp->sp_reserved = le16_to_cpu(cmd.reserved);
 305     }
 306 
 307     lbs_deb_leave_args(LBS_DEB_CMD, "ret %d", ret);
 308     return 0;              // return ret?
 309 }

lbs_sleepparams_read @@ drivers/net/wireless/marvell/libertas/debugfs.c
 91 static ssize_t lbs_sleepparams_read(struct file *file, char __user *userbuf,
 92                                   size_t count, loff_t *ppos)
 93 {
 94         struct lbs_private *priv = file->private_data;
 95         ssize_t ret;
 96         size_t pos = 0;
 97         struct sleep_params sp;
 98         unsigned long addr = get_zeroed_page(GFP_KERNEL);
 99         char *buf = (char *)addr;
100         if (!buf)
101                 return -ENOMEM;
102 
103         ret = lbs_cmd_802_11_sleep_params(priv, CMD_ACT_GET, &sp);
104         if (ret)
105                 goto out_unlock;
106 
            // sp may be uninitialized
107         pos += snprintf(buf, len, "%d %d %d %d %d %d\n", sp.sp_error,
108                         sp.sp_offset, sp.sp_stabletime,
109                         sp.sp_calcontrol, sp.sp_extsleepclk,
110                         sp.sp_reserved);
111 
112         ret = simple_read_from_buffer(userbuf, count, ppos, buf, pos);
113 
114 out_unlock:
115         free_page(addr);
116         return ret;
117 }

Thanks very much!
Comment 1 bianpan 2017-05-11 09:51:29 UTC
Created attachment 256391 [details]
The patch fixes the bug

The patch has been merged into the latest version of the Linux kernel. So I will close it.

Note You need to log in before you can comment on or make changes to this bug.