* [bug report] libnvdimm: control (ioctl) messages for nvdimm_bus and nvdimm devices
@ 2019-04-24 9:48 Dan Carpenter
0 siblings, 0 replies; 2+ messages in thread
From: Dan Carpenter @ 2019-04-24 9:48 UTC (permalink / raw)
To: dan.j.williams; +Cc: linux-acpi
Hello Dan Williams,
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/acpi/nfit/core.c:611 acpi_nfit_ctl()
error: 'out_size' from user is not capped properly
drivers/acpi/nfit/core.c
594 for (i = 0, offset = 0; i < desc->out_num; i++) {
595 u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i, buf,
596 (u32 *) out_obj->buffer.pointer,
597 out_obj->buffer.length - offset);
598
599 if (offset + out_size > out_obj->buffer.length) {
^^^^^^^^^^^^^^^^^
It looks like this addition could have an integer overflow bug.
600 dev_dbg(dev, "%s output object underflow cmd: %s field: %d\n",
601 dimm_name, cmd_name, i);
602 break;
603 }
604
605 if (in_buf.buffer.length + offset + out_size > buf_len) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The comments suggest that this is safe. I don't know the code.
606 dev_dbg(dev, "%s output overrun cmd: %s field: %d\n",
607 dimm_name, cmd_name, i);
608 rc = -ENXIO;
609 goto out;
610 }
611 memcpy(buf + in_buf.buffer.length + offset,
612 out_obj->buffer.pointer + offset, out_size);
613 offset += out_size;
614 }
615
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* [bug report] libnvdimm: control (ioctl) messages for nvdimm_bus and nvdimm devices
@ 2017-07-27 15:14 Dan Carpenter
0 siblings, 0 replies; 2+ messages in thread
From: Dan Carpenter @ 2017-07-27 15:14 UTC (permalink / raw)
To: dan.j.williams; +Cc: linux-nvdimm
Hello Dan Williams,
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'
drivers/nvdimm/bus.c
959 /* process an input envelope */
960 for (i = 0; i < desc->in_num; i++) {
961 u32 in_size, copy;
962
963 in_size = nd_cmd_in_size(nvdimm, cmd, desc, i, in_env);
964 if (in_size == UINT_MAX) {
965 dev_err(dev, "%s:%s unknown input size cmd: %s field: %d\n",
966 __func__, dimm_name, cmd_name, i);
967 return -ENXIO;
968 }
969 if (in_len < sizeof(in_env))
970 copy = min_t(u32, sizeof(in_env) - in_len, in_size);
971 else
972 copy = 0;
973 if (copy && copy_from_user(&in_env[in_len], p + in_len, copy))
974 return -EFAULT;
975 in_len += in_size;
>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:
- in_len += in_size;
+ in_len += copy;
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.
976 }
977
978 if (cmd == ND_CMD_CALL) {
979 func = pkg.nd_command;
980 dev_dbg(dev, "%s:%s, idx: %llu, in: %zu, out: %zu, len %zu\n",
981 __func__, dimm_name, pkg.nd_command,
982 in_len, out_len, buf_len);
983
984 for (i = 0; i < ARRAY_SIZE(pkg.nd_reserved2); i++)
985 if (pkg.nd_reserved2[i])
986 return -EINVAL;
987 }
988
989 /* process an output envelope */
990 for (i = 0; i < desc->out_num; i++) {
991 u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
992 (u32 *) in_env, (u32 *) out_env, 0);
993 u32 copy;
994
995 if (out_size == UINT_MAX) {
996 dev_dbg(dev, "%s:%s unknown output size cmd: %s field: %d\n",
997 __func__, dimm_name, cmd_name, i);
998 return -EFAULT;
999 }
1000 if (out_len < sizeof(out_env))
1001 copy = min_t(u32, sizeof(out_env) - out_len, out_size);
1002 else
1003 copy = 0;
1004 if (copy && copy_from_user(&out_env[out_len],
1005 p + in_len + out_len, copy))
1006 return -EFAULT;
1007 out_len += out_size;
Same thing.
1008 }
1009
1010 buf_len = out_len + in_len;
It means this addition could overflow.
1011 if (buf_len > ND_IOCTL_MAX_BUFLEN) {
1012 dev_dbg(dev, "%s:%s cmd: %s buf_len: %zu > %d\n", __func__,
1013 dimm_name, cmd_name, buf_len,
1014 ND_IOCTL_MAX_BUFLEN);
1015 return -EINVAL;
1016 }
1017
1018 buf = vmalloc(buf_len);
^^^^^^^
The checker complains we might allocate less than intended.
1019 if (!buf)
1020 return -ENOMEM;
1021
1022 if (copy_from_user(buf, p, buf_len)) {
1023 rc = -EFAULT;
1024 goto out;
1025 }
regards,
dan carpenter
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-04-24 9:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-24 9:48 [bug report] libnvdimm: control (ioctl) messages for nvdimm_bus and nvdimm devices Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2017-07-27 15:14 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.