| Message ID | 20160113053934.6293.5029.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Tue, Jan 12, 2016 at 09:39:34PM -0800, Dan Williams wrote: > Prevent invalid commands from being sent to ACPICA, check the requested > dsm function number against the support dsm mask. Besides protecting > against known invalid commands this also enables the kernel to guarantee > it has exclusive access to manipulate namespace labels (for buses that > implement get_config and set_config commands). > > A side effect of this change is that the kernel must know the uuid of > the device in advance so that it can populate a correct dsm_mask. The > top-level bus device has a known uuid, but the per-dimm device are > format interface code specific. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- ... > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > index 6e9787ad1fe1..b86d5eda78ed 100644 > --- a/drivers/nvdimm/bus.c > +++ b/drivers/nvdimm/bus.c > @@ -512,35 +512,38 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, > unsigned int cmd = _IOC_NR(ioctl_cmd); > void __user *p = (void __user *) arg; > struct device *dev = &nvdimm_bus->dev; > + struct nd_cmd_dsmcall_pkg call_dsm; > const char *cmd_name, *dimm_name; > - unsigned long dsm_mask = ~0UL; > + unsigned int func = cmd; > + unsigned long dsm_mask; > void *buf; > int rc, i; > > - struct nd_cmd_dsmcall_pkg *pkg; > - int dsm_call = (cmd == ND_CMD_CALL_DSM); > - > if (nvdimm) { > desc = nd_cmd_dimm_desc(cmd); > cmd_name = nvdimm_cmd_name(cmd); > - if (!dsm_call) > - dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; > + dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; > dimm_name = dev_name(&nvdimm->dev); > } else { > desc = nd_cmd_bus_desc(cmd); > cmd_name = nvdimm_bus_cmd_name(cmd); > - if (!dsm_call) > - dsm_mask = nd_desc->dsm_mask; > + dsm_mask = nd_desc->dsm_mask; > dimm_name = "bus"; > } > > + if (cmd == ND_CMD_CALL_DSM) { > + if (copy_from_user(&call_dsm, p, sizeof(cmd))) BUG: want sizeof(call_dsm).
On Wed, Jan 13, 2016 at 6:53 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Tue, Jan 12, 2016 at 09:39:34PM -0800, Dan Williams wrote: >> Prevent invalid commands from being sent to ACPICA, check the requested >> dsm function number against the support dsm mask. Besides protecting >> against known invalid commands this also enables the kernel to guarantee >> it has exclusive access to manipulate namespace labels (for buses that >> implement get_config and set_config commands). >> >> A side effect of this change is that the kernel must know the uuid of >> the device in advance so that it can populate a correct dsm_mask. The >> top-level bus device has a known uuid, but the per-dimm device are >> format interface code specific. >> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- > > > ... > > >> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c >> index 6e9787ad1fe1..b86d5eda78ed 100644 >> --- a/drivers/nvdimm/bus.c >> +++ b/drivers/nvdimm/bus.c >> @@ -512,35 +512,38 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, >> unsigned int cmd = _IOC_NR(ioctl_cmd); >> void __user *p = (void __user *) arg; >> struct device *dev = &nvdimm_bus->dev; >> + struct nd_cmd_dsmcall_pkg call_dsm; >> const char *cmd_name, *dimm_name; >> - unsigned long dsm_mask = ~0UL; >> + unsigned int func = cmd; >> + unsigned long dsm_mask; >> void *buf; >> int rc, i; >> >> - struct nd_cmd_dsmcall_pkg *pkg; >> - int dsm_call = (cmd == ND_CMD_CALL_DSM); >> - >> if (nvdimm) { >> desc = nd_cmd_dimm_desc(cmd); >> cmd_name = nvdimm_cmd_name(cmd); >> - if (!dsm_call) >> - dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; >> + dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; >> dimm_name = dev_name(&nvdimm->dev); >> } else { >> desc = nd_cmd_bus_desc(cmd); >> cmd_name = nvdimm_bus_cmd_name(cmd); >> - if (!dsm_call) >> - dsm_mask = nd_desc->dsm_mask; >> + dsm_mask = nd_desc->dsm_mask; >> dimm_name = "bus"; >> } >> >> + if (cmd == ND_CMD_CALL_DSM) { >> + if (copy_from_user(&call_dsm, p, sizeof(cmd))) > > > BUG: want sizeof(call_dsm). Yup, good eye.
On Tue, Jan 12, 2016 at 09:39:34PM -0800, Dan Williams wrote: > Prevent invalid commands from being sent to ACPICA, check the requested > dsm function number against the support dsm mask. Besides protecting > against known invalid commands this also enables the kernel to guarantee > it has exclusive access to manipulate namespace labels (for buses that > implement get_config and set_config commands). > > A side effect of this change is that the kernel must know the uuid of > the device in advance so that it can populate a correct dsm_mask. The > top-level bus device has a known uuid, but the per-dimm device are > format interface code specific. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/acpi/nfit.c | 39 ++++++++++++++++++++++----------------- > drivers/nvdimm/bus.c | 35 ++++++++++++++++++----------------- > 2 files changed, 40 insertions(+), 34 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 9ba5a9a4b943..00aa20adde7c 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -76,8 +76,9 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, > unsigned int buf_len) > { > struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc); > - const struct nd_cmd_desc *desc = NULL; > union acpi_object in_obj, in_buf, *out_obj; > + struct nd_cmd_dsmcall_pkg *call_dsm = NULL; > + const struct nd_cmd_desc *desc = NULL; > struct device *dev = acpi_desc->dev; > const char *cmd_name, *dimm_name; > unsigned long dsm_mask; > @@ -87,8 +88,10 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, > int rc, i; > __u64 rev = 1, func = cmd; > > - struct nd_cmd_dsmcall_pkg *pkg = buf; > - int dsm_call = (cmd == ND_CMD_CALL_DSM); > + if (cmd == ND_CMD_CALL_DSM) { > + call_dsm = buf; > + func = call_dsm->dsm_fun_idx; > + } > > if (nvdimm) { > struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm); > @@ -112,13 +115,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, > handle = adev->handle; > dimm_name = "bus"; > } > - if (dsm_call) > - dsm_mask = ~0UL; > > if (!desc || (cmd && (desc->out_num + desc->in_num == 0))) > return -ENOTTY; > > - if (!test_bit(cmd, &dsm_mask)) > + if (!test_bit(func, &dsm_mask)) > return -ENOTTY; Note: we also test against dsm_mask in __nd_ioctl. > > in_obj.type = ACPI_TYPE_PACKAGE; > @@ -133,14 +134,16 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, > in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc, > i, buf); > > - if (dsm_call) { > + if (call_dsm) { > /* must skip over package wrapper */ > - in_buf.buffer.pointer = (void *) &pkg->dsm_buf; > - in_buf.buffer.length = pkg->dsm_in; > - /* for pass thru must use value sent in from user space. */ > - uuid = pkg->dsm_uuid; > - rev = pkg->dsm_rev; > - func = pkg->dsm_fun_idx; > + in_buf.buffer.pointer = (void *) &call_dsm->dsm_buf; > + in_buf.buffer.length = call_dsm->dsm_in; > + if (memcmp(call_dsm->dsm_uuid, uuid, 16) != 0) { 16? suggest sizeof(call_dsm->dsm_uuid) > + dev_dbg(dev, "%s:%s unsupported uuid\n", dimm_name, > + cmd_name); > + return -EINVAL; > + } > + rev = call_dsm->dsm_rev; > } > > if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) { > @@ -173,10 +176,12 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, > min_t(u32, 256, out_obj->buffer.length), true); > } > > - if (dsm_call) { > - pkg->dsm_size = out_obj->buffer.length; > - memcpy(pkg->dsm_buf + pkg->dsm_in, out_obj->buffer.pointer, > - min(pkg->dsm_size, pkg->dsm_out)); > + if (call_dsm) { > + call_dsm->dsm_size = out_obj->buffer.length; > + memcpy(call_dsm->dsm_buf + call_dsm->dsm_in, > + out_obj->buffer.pointer, > + min(call_dsm->dsm_size, call_dsm->dsm_out)); > + > ACPI_FREE(out_obj); > return 0; > } > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > index 6e9787ad1fe1..b86d5eda78ed 100644 > --- a/drivers/nvdimm/bus.c > +++ b/drivers/nvdimm/bus.c > @@ -512,35 +512,38 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, > unsigned int cmd = _IOC_NR(ioctl_cmd); > void __user *p = (void __user *) arg; > struct device *dev = &nvdimm_bus->dev; > + struct nd_cmd_dsmcall_pkg call_dsm; > const char *cmd_name, *dimm_name; > - unsigned long dsm_mask = ~0UL; > + unsigned int func = cmd; > + unsigned long dsm_mask; > void *buf; > int rc, i; > > - struct nd_cmd_dsmcall_pkg *pkg; > - int dsm_call = (cmd == ND_CMD_CALL_DSM); > - > if (nvdimm) { > desc = nd_cmd_dimm_desc(cmd); > cmd_name = nvdimm_cmd_name(cmd); > - if (!dsm_call) > - dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; > + dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; > dimm_name = dev_name(&nvdimm->dev); > } else { > desc = nd_cmd_bus_desc(cmd); > cmd_name = nvdimm_bus_cmd_name(cmd); > - if (!dsm_call) > - dsm_mask = nd_desc->dsm_mask; > + dsm_mask = nd_desc->dsm_mask; > dimm_name = "bus"; > } > > + if (cmd == ND_CMD_CALL_DSM) { > + if (copy_from_user(&call_dsm, p, sizeof(cmd))) Previously reported want sizeof(call_dsm) Note: the code copies the same data in from user space three times. This new instance the copy is to be able to func index to test against dsm_mask. The test against dsm_mask could be moved to later in the function to avoid this new copy_from_user. acpi_nfit_ctl also tests against dsm_mask. can we drop the dsm_mask test here and just use the one in acpi_nfit_ctl? > + return -EFAULT; > + func = call_dsm.dsm_fun_idx; > + } > + > if (!desc || (desc->out_num + desc->in_num == 0) || > - !test_bit(cmd, &dsm_mask)) > + !test_bit(func, &dsm_mask)) > return -ENOTTY; > > /* fail write commands (when read-only) */ > if (read_only) > - switch (cmd) { > + switch (func) { BUG: revert want cmd. if ND_CMD_CALL_DSM func is set to call_dsm.dsm_fun_idx; this could be a DSM w/ totally different semantics. > case ND_CMD_VENDOR: > case ND_CMD_SET_CONFIG_DATA: > case ND_CMD_ARS_START: > @@ -572,15 +575,13 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, > in_len += in_size; > } > > - if (dsm_call) { > - pkg = (struct nd_cmd_dsmcall_pkg *) in_env; > - > + if (cmd == ND_CMD_CALL_DSM) { > dev_dbg(dev, "%s:%s rev: %llu, idx: %llu, in: %zu, out: %zu, len %zu\n", > - __func__, dimm_name, pkg->dsm_rev, > - pkg->dsm_fun_idx, in_len, out_len, buf_len); > + __func__, dimm_name, call_dsm.dsm_rev, > + call_dsm.dsm_fun_idx, in_len, out_len, buf_len); > > - for (i = 0; i < ARRAY_SIZE(pkg->reserved2); i++) > - if (pkg->reserved2[i]) > + for (i = 0; i < ARRAY_SIZE(call_dsm.reserved2); i++) > + if (call_dsm.reserved2[i]) > return -EINVAL; > } > Note, if you take move test on dsm_mask above later and don't do the additional copy_from_user, then this change would be affected.
On Thu, Jan 14, 2016 at 3:00 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Tue, Jan 12, 2016 at 09:39:34PM -0800, Dan Williams wrote: >> Prevent invalid commands from being sent to ACPICA, check the requested >> dsm function number against the support dsm mask. Besides protecting >> against known invalid commands this also enables the kernel to guarantee >> it has exclusive access to manipulate namespace labels (for buses that >> implement get_config and set_config commands). >> >> A side effect of this change is that the kernel must know the uuid of >> the device in advance so that it can populate a correct dsm_mask. The >> top-level bus device has a known uuid, but the per-dimm device are >> format interface code specific. >> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/acpi/nfit.c | 39 ++++++++++++++++++++++----------------- >> drivers/nvdimm/bus.c | 35 ++++++++++++++++++----------------- >> 2 files changed, 40 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c >> index 9ba5a9a4b943..00aa20adde7c 100644 >> --- a/drivers/acpi/nfit.c >> +++ b/drivers/acpi/nfit.c >> @@ -76,8 +76,9 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, >> unsigned int buf_len) >> { >> struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc); >> - const struct nd_cmd_desc *desc = NULL; >> union acpi_object in_obj, in_buf, *out_obj; >> + struct nd_cmd_dsmcall_pkg *call_dsm = NULL; >> + const struct nd_cmd_desc *desc = NULL; >> struct device *dev = acpi_desc->dev; >> const char *cmd_name, *dimm_name; >> unsigned long dsm_mask; >> @@ -87,8 +88,10 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, >> int rc, i; >> __u64 rev = 1, func = cmd; >> >> - struct nd_cmd_dsmcall_pkg *pkg = buf; >> - int dsm_call = (cmd == ND_CMD_CALL_DSM); >> + if (cmd == ND_CMD_CALL_DSM) { >> + call_dsm = buf; >> + func = call_dsm->dsm_fun_idx; >> + } >> >> if (nvdimm) { >> struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm); >> @@ -112,13 +115,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, >> handle = adev->handle; >> dimm_name = "bus"; >> } >> - if (dsm_call) >> - dsm_mask = ~0UL; >> >> if (!desc || (cmd && (desc->out_num + desc->in_num == 0))) >> return -ENOTTY; >> >> - if (!test_bit(cmd, &dsm_mask)) >> + if (!test_bit(func, &dsm_mask)) >> return -ENOTTY; > > > Note: we also test against dsm_mask in __nd_ioctl. Yes, see below... > > > >> >> in_obj.type = ACPI_TYPE_PACKAGE; >> @@ -133,14 +134,16 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, >> in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc, >> i, buf); >> >> - if (dsm_call) { >> + if (call_dsm) { >> /* must skip over package wrapper */ >> - in_buf.buffer.pointer = (void *) &pkg->dsm_buf; >> - in_buf.buffer.length = pkg->dsm_in; >> - /* for pass thru must use value sent in from user space. */ >> - uuid = pkg->dsm_uuid; >> - rev = pkg->dsm_rev; >> - func = pkg->dsm_fun_idx; >> + in_buf.buffer.pointer = (void *) &call_dsm->dsm_buf; >> + in_buf.buffer.length = call_dsm->dsm_in; >> + if (memcmp(call_dsm->dsm_uuid, uuid, 16) != 0) { > > > 16? suggest sizeof(call_dsm->dsm_uuid) Sure. > > > >> + dev_dbg(dev, "%s:%s unsupported uuid\n", dimm_name, >> + cmd_name); >> + return -EINVAL; >> + } >> + rev = call_dsm->dsm_rev; >> } >> >> if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) { >> @@ -173,10 +176,12 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, >> min_t(u32, 256, out_obj->buffer.length), true); >> } >> >> - if (dsm_call) { >> - pkg->dsm_size = out_obj->buffer.length; >> - memcpy(pkg->dsm_buf + pkg->dsm_in, out_obj->buffer.pointer, >> - min(pkg->dsm_size, pkg->dsm_out)); >> + if (call_dsm) { >> + call_dsm->dsm_size = out_obj->buffer.length; >> + memcpy(call_dsm->dsm_buf + call_dsm->dsm_in, >> + out_obj->buffer.pointer, >> + min(call_dsm->dsm_size, call_dsm->dsm_out)); >> + >> ACPI_FREE(out_obj); >> return 0; >> } >> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c >> index 6e9787ad1fe1..b86d5eda78ed 100644 >> --- a/drivers/nvdimm/bus.c >> +++ b/drivers/nvdimm/bus.c >> @@ -512,35 +512,38 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, >> unsigned int cmd = _IOC_NR(ioctl_cmd); >> void __user *p = (void __user *) arg; >> struct device *dev = &nvdimm_bus->dev; >> + struct nd_cmd_dsmcall_pkg call_dsm; >> const char *cmd_name, *dimm_name; >> - unsigned long dsm_mask = ~0UL; >> + unsigned int func = cmd; >> + unsigned long dsm_mask; >> void *buf; >> int rc, i; >> >> - struct nd_cmd_dsmcall_pkg *pkg; >> - int dsm_call = (cmd == ND_CMD_CALL_DSM); >> - >> if (nvdimm) { >> desc = nd_cmd_dimm_desc(cmd); >> cmd_name = nvdimm_cmd_name(cmd); >> - if (!dsm_call) >> - dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; >> + dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; >> dimm_name = dev_name(&nvdimm->dev); >> } else { >> desc = nd_cmd_bus_desc(cmd); >> cmd_name = nvdimm_bus_cmd_name(cmd); >> - if (!dsm_call) >> - dsm_mask = nd_desc->dsm_mask; >> + dsm_mask = nd_desc->dsm_mask; >> dimm_name = "bus"; >> } >> >> + if (cmd == ND_CMD_CALL_DSM) { >> + if (copy_from_user(&call_dsm, p, sizeof(cmd))) > > Previously reported want sizeof(call_dsm) Yup, as you can see this was definitely RFC material. > > Note: the code copies the same data in from user space three times. > > This new instance the copy is to be able to func index to test > against dsm_mask. The test against dsm_mask could be moved to > later in the function to avoid this new copy_from_user. > > acpi_nfit_ctl also tests against dsm_mask. can we drop the > dsm_mask test here and just use the one in acpi_nfit_ctl? This speaks to the fact that the ioctl path is currently overloaded in the core for all bus types. So far all the bus types that support ioctls just happen to be nfit-based, but that need not always be the case. The eventual bus that comes along and wants ioctl support will need to suffer with the fact that some ACPI terms have leaked into the interface vocabulary. > > > >> + return -EFAULT; >> + func = call_dsm.dsm_fun_idx; >> + } >> + >> if (!desc || (desc->out_num + desc->in_num == 0) || >> - !test_bit(cmd, &dsm_mask)) >> + !test_bit(func, &dsm_mask)) >> return -ENOTTY; >> >> /* fail write commands (when read-only) */ >> if (read_only) >> - switch (cmd) { >> + switch (func) { > > > BUG: revert want cmd. if ND_CMD_CALL_DSM func is set to call_dsm.dsm_fun_idx; > this could be a DSM w/ totally different semantics. > When we add support for other UUIDs we will need to re-evaluate the set of known "write" function ids. I suspect if the device is read-only we should fail any unknown command. > > > >> case ND_CMD_VENDOR: >> case ND_CMD_SET_CONFIG_DATA: >> case ND_CMD_ARS_START: >> @@ -572,15 +575,13 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, >> in_len += in_size; >> } >> >> - if (dsm_call) { >> - pkg = (struct nd_cmd_dsmcall_pkg *) in_env; >> - > > > >> + if (cmd == ND_CMD_CALL_DSM) { >> dev_dbg(dev, "%s:%s rev: %llu, idx: %llu, in: %zu, out: %zu, len %zu\n", >> - __func__, dimm_name, pkg->dsm_rev, >> - pkg->dsm_fun_idx, in_len, out_len, buf_len); >> + __func__, dimm_name, call_dsm.dsm_rev, >> + call_dsm.dsm_fun_idx, in_len, out_len, buf_len); >> >> - for (i = 0; i < ARRAY_SIZE(pkg->reserved2); i++) >> - if (pkg->reserved2[i]) >> + for (i = 0; i < ARRAY_SIZE(call_dsm.reserved2); i++) >> + if (call_dsm.reserved2[i]) >> return -EINVAL; >> } >> > > Note, if you take move test on dsm_mask above later and don't do > the additional copy_from_user, then this change would be affected. > > Thanks for the review. Side note, we need a unit test for this functionality. My current starting is to just convert the existing tests over to the generic envelope, but other tests are welcome too if you want to contribute.
On Tue, Jan 12, 2016 at 09:39:34PM -0800, Dan Williams wrote: > Prevent invalid commands from being sent to ACPICA, check the requested > dsm function number against the support dsm mask. Besides protecting > against known invalid commands this also enables the kernel to guarantee > it has exclusive access to manipulate namespace labels (for buses that > implement get_config and set_config commands). > The get_config, set_config suggests a read-modify-write semantic that would imply locking across both calls. Are the tools using it in a R-M-W fashion? How are they handling the locking? ----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise -----------------------------------------------------------------------------
On Thu, Jan 14, 2016 at 5:23 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Tue, Jan 12, 2016 at 09:39:34PM -0800, Dan Williams wrote: >> Prevent invalid commands from being sent to ACPICA, check the requested >> dsm function number against the support dsm mask. Besides protecting >> against known invalid commands this also enables the kernel to guarantee >> it has exclusive access to manipulate namespace labels (for buses that >> implement get_config and set_config commands). >> > The get_config, set_config suggests a read-modify-write semantic > that would imply locking across both calls. > > Are the tools using it in a R-M-W fashion? How are they handling > the locking? When the label area is in use by the kernel it blocks out userspace writes and operates under the nvdimm_bus_lock. When the device is idle the kernel doesn't care if userspace races itself. It will take the lock and validate the result when the device is enabled.
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 9ba5a9a4b943..00aa20adde7c 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -76,8 +76,9 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, unsigned int buf_len) { struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc); - const struct nd_cmd_desc *desc = NULL; union acpi_object in_obj, in_buf, *out_obj; + struct nd_cmd_dsmcall_pkg *call_dsm = NULL; + const struct nd_cmd_desc *desc = NULL; struct device *dev = acpi_desc->dev; const char *cmd_name, *dimm_name; unsigned long dsm_mask; @@ -87,8 +88,10 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, int rc, i; __u64 rev = 1, func = cmd; - struct nd_cmd_dsmcall_pkg *pkg = buf; - int dsm_call = (cmd == ND_CMD_CALL_DSM); + if (cmd == ND_CMD_CALL_DSM) { + call_dsm = buf; + func = call_dsm->dsm_fun_idx; + } if (nvdimm) { struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm); @@ -112,13 +115,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, handle = adev->handle; dimm_name = "bus"; } - if (dsm_call) - dsm_mask = ~0UL; if (!desc || (cmd && (desc->out_num + desc->in_num == 0))) return -ENOTTY; - if (!test_bit(cmd, &dsm_mask)) + if (!test_bit(func, &dsm_mask)) return -ENOTTY; in_obj.type = ACPI_TYPE_PACKAGE; @@ -133,14 +134,16 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc, i, buf); - if (dsm_call) { + if (call_dsm) { /* must skip over package wrapper */ - in_buf.buffer.pointer = (void *) &pkg->dsm_buf; - in_buf.buffer.length = pkg->dsm_in; - /* for pass thru must use value sent in from user space. */ - uuid = pkg->dsm_uuid; - rev = pkg->dsm_rev; - func = pkg->dsm_fun_idx; + in_buf.buffer.pointer = (void *) &call_dsm->dsm_buf; + in_buf.buffer.length = call_dsm->dsm_in; + if (memcmp(call_dsm->dsm_uuid, uuid, 16) != 0) { + dev_dbg(dev, "%s:%s unsupported uuid\n", dimm_name, + cmd_name); + return -EINVAL; + } + rev = call_dsm->dsm_rev; } if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) { @@ -173,10 +176,12 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, min_t(u32, 256, out_obj->buffer.length), true); } - if (dsm_call) { - pkg->dsm_size = out_obj->buffer.length; - memcpy(pkg->dsm_buf + pkg->dsm_in, out_obj->buffer.pointer, - min(pkg->dsm_size, pkg->dsm_out)); + if (call_dsm) { + call_dsm->dsm_size = out_obj->buffer.length; + memcpy(call_dsm->dsm_buf + call_dsm->dsm_in, + out_obj->buffer.pointer, + min(call_dsm->dsm_size, call_dsm->dsm_out)); + ACPI_FREE(out_obj); return 0; } diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index 6e9787ad1fe1..b86d5eda78ed 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -512,35 +512,38 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, unsigned int cmd = _IOC_NR(ioctl_cmd); void __user *p = (void __user *) arg; struct device *dev = &nvdimm_bus->dev; + struct nd_cmd_dsmcall_pkg call_dsm; const char *cmd_name, *dimm_name; - unsigned long dsm_mask = ~0UL; + unsigned int func = cmd; + unsigned long dsm_mask; void *buf; int rc, i; - struct nd_cmd_dsmcall_pkg *pkg; - int dsm_call = (cmd == ND_CMD_CALL_DSM); - if (nvdimm) { desc = nd_cmd_dimm_desc(cmd); cmd_name = nvdimm_cmd_name(cmd); - if (!dsm_call) - dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; + dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0; dimm_name = dev_name(&nvdimm->dev); } else { desc = nd_cmd_bus_desc(cmd); cmd_name = nvdimm_bus_cmd_name(cmd); - if (!dsm_call) - dsm_mask = nd_desc->dsm_mask; + dsm_mask = nd_desc->dsm_mask; dimm_name = "bus"; } + if (cmd == ND_CMD_CALL_DSM) { + if (copy_from_user(&call_dsm, p, sizeof(cmd))) + return -EFAULT; + func = call_dsm.dsm_fun_idx; + } + if (!desc || (desc->out_num + desc->in_num == 0) || - !test_bit(cmd, &dsm_mask)) + !test_bit(func, &dsm_mask)) return -ENOTTY; /* fail write commands (when read-only) */ if (read_only) - switch (cmd) { + switch (func) { case ND_CMD_VENDOR: case ND_CMD_SET_CONFIG_DATA: case ND_CMD_ARS_START: @@ -572,15 +575,13 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, in_len += in_size; } - if (dsm_call) { - pkg = (struct nd_cmd_dsmcall_pkg *) in_env; - + if (cmd == ND_CMD_CALL_DSM) { dev_dbg(dev, "%s:%s rev: %llu, idx: %llu, in: %zu, out: %zu, len %zu\n", - __func__, dimm_name, pkg->dsm_rev, - pkg->dsm_fun_idx, in_len, out_len, buf_len); + __func__, dimm_name, call_dsm.dsm_rev, + call_dsm.dsm_fun_idx, in_len, out_len, buf_len); - for (i = 0; i < ARRAY_SIZE(pkg->reserved2); i++) - if (pkg->reserved2[i]) + for (i = 0; i < ARRAY_SIZE(call_dsm.reserved2); i++) + if (call_dsm.reserved2[i]) return -EINVAL; }
Prevent invalid commands from being sent to ACPICA, check the requested dsm function number against the support dsm mask. Besides protecting against known invalid commands this also enables the kernel to guarantee it has exclusive access to manipulate namespace labels (for buses that implement get_config and set_config commands). A side effect of this change is that the kernel must know the uuid of the device in advance so that it can populate a correct dsm_mask. The top-level bus device has a known uuid, but the per-dimm device are format interface code specific. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/acpi/nfit.c | 39 ++++++++++++++++++++++----------------- drivers/nvdimm/bus.c | 35 ++++++++++++++++++----------------- 2 files changed, 40 insertions(+), 34 deletions(-)