| From foo@baz Mon Feb 26 20:55:53 CET 2018 |
| From: Dan Williams <dan.j.williams@intel.com> |
| Date: Fri, 23 Feb 2018 14:05:38 -0800 |
| Subject: libnvdimm: fix integer overflow static analysis warning |
| To: gregkh@linuxfoundation.org |
| Cc: stable@vger.kernel.org, Dan Carpenter <dan.carpenter@oracle.com>, linux-kernel@vger.kernel.org |
| Message-ID: <151942353841.21775.10479863744600514056.stgit@dwillia2-desk3.amr.corp.intel.com> |
| |
| From: Dan Williams <dan.j.williams@intel.com> |
| |
| commit 58738c495e15badd2015e19ff41f1f1ed55200bc upstream. |
| |
| Dan reports: |
| The patch 62232e45f4a2: "libnvdimm: control (ioctl) messages for |
| nvdimm_bus and nvdimm devices" from Jun 8, 2015, leads to the |
| following static checker warning: |
| |
| drivers/nvdimm/bus.c:1018 __nd_ioctl() |
| warn: integer overflows 'buf_len' |
| |
| From a casual review, this seems like it might be a real bug. On |
| the first iteration we load some data into in_env[]. On the second |
| iteration we read a use controlled "in_size" from nd_cmd_in_size(). |
| It can go up to UINT_MAX - 1. A high number means we will fill the |
| whole in_env[] buffer. But we potentially keep looping and adding |
| more to in_len so now it can be any value. |
| |
| It simple enough to change, but it feels weird that we keep looping |
| even though in_env is totally full. Shouldn't we just return an |
| error if we don't have space for desc->in_num. |
| |
| We keep looping because the size of the total input is allowed to be |
| bigger than the 'envelope' which is a subset of the payload that tells |
| us how much data to expect. For safety explicitly check that buf_len |
| does not overflow which is what the checker flagged. |
| |
| Cc: <stable@vger.kernel.org> |
| Fixes: 62232e45f4a2: "libnvdimm: control (ioctl) messages for nvdimm_bus..." |
| Reported-by: Dan Carpenter <dan.carpenter@oracle.com> |
| Signed-off-by: Dan Williams <dan.j.williams@intel.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/nvdimm/bus.c | 11 ++++++----- |
| 1 file changed, 6 insertions(+), 5 deletions(-) |
| |
| --- a/drivers/nvdimm/bus.c |
| +++ b/drivers/nvdimm/bus.c |
| @@ -812,16 +812,17 @@ static int __nd_ioctl(struct nvdimm_bus |
| int read_only, unsigned int ioctl_cmd, unsigned long arg) |
| { |
| struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc; |
| - size_t buf_len = 0, in_len = 0, out_len = 0; |
| static char out_env[ND_CMD_MAX_ENVELOPE]; |
| static char in_env[ND_CMD_MAX_ENVELOPE]; |
| const struct nd_cmd_desc *desc = NULL; |
| unsigned int cmd = _IOC_NR(ioctl_cmd); |
| void __user *p = (void __user *) arg; |
| struct device *dev = &nvdimm_bus->dev; |
| - struct nd_cmd_pkg pkg; |
| const char *cmd_name, *dimm_name; |
| + u32 in_len = 0, out_len = 0; |
| unsigned long cmd_mask; |
| + struct nd_cmd_pkg pkg; |
| + u64 buf_len = 0; |
| void *buf; |
| int rc, i; |
| |
| @@ -882,7 +883,7 @@ static int __nd_ioctl(struct nvdimm_bus |
| } |
| |
| if (cmd == ND_CMD_CALL) { |
| - dev_dbg(dev, "%s:%s, idx: %llu, in: %zu, out: %zu, len %zu\n", |
| + dev_dbg(dev, "%s:%s, idx: %llu, in: %u, out: %u, len %llu\n", |
| __func__, dimm_name, pkg.nd_command, |
| in_len, out_len, buf_len); |
| |
| @@ -912,9 +913,9 @@ static int __nd_ioctl(struct nvdimm_bus |
| out_len += out_size; |
| } |
| |
| - buf_len = out_len + in_len; |
| + buf_len = (u64) out_len + (u64) in_len; |
| if (buf_len > ND_IOCTL_MAX_BUFLEN) { |
| - dev_dbg(dev, "%s:%s cmd: %s buf_len: %zu > %d\n", __func__, |
| + dev_dbg(dev, "%s:%s cmd: %s buf_len: %llu > %d\n", __func__, |
| dimm_name, cmd_name, buf_len, |
| ND_IOCTL_MAX_BUFLEN); |
| return -EINVAL; |