On Tue, Jan 15, 2019 at 2:46 PM Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
> The _DSM function number validation only happens to succeed when the
> generic Linux command number translation corresponds with a
> DSM-family-specific function number. This breaks NVDIMM-N
> implementations that correctly implement _LSR, _LSW, and _LSI, but do
> not happen to publish support for DSM function numbers 4, 5, and 6.
>
> Recall that the support for _LS{I,R,W} family of methods results in the
> DIMM being marked as supporting those command numbers at
> acpi_nfit_register_dimms() time. The DSM function mask is only used for
> ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
>
> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command...")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Link: https://github.com/pmem/ndctl/issues/78
> Reported-by: Sujith Pandel <sujith_pandel@xxxxxxxx>
> Tested-by: Sujith Pandel <sujith_pandel@xxxxxxxx>
> Reviewed-by: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> Reviewed-by: Jeff Moyer <jmoyer@xxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> drivers/acpi/nfit/core.c | 52 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 73281b19d3dd..9c95b82e5e5d 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -409,6 +409,32 @@ static bool payload_dumpable(struct nvdimm *nvdimm, unsigned int func)
> return true;
> }
>
> +static int cmd_to_func(struct nfit_mem *nfit_mem, unsigned int cmd,
> + struct nd_cmd_pkg *call_pkg)
> +{
> + if (cmd == ND_CMD_CALL) {
> + int i;
> +
> + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
> + return -ENOTTY;
> +
> + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
> + if (call_pkg->nd_reserved2[i])
> + return -EINVAL;
> + return call_pkg->nd_command;
> + }
> +
> + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
> + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
> + return cmd;
> +
> + /*
> + * Force function number validation to fail since 0 is never
> + * published as a valid function in dsm_mask.
> + */
> + return 0;
> +}
> +
> int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
> {
> @@ -422,30 +448,21 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> unsigned long cmd_mask, dsm_mask;
> u32 offset, fw_status = 0;
> acpi_handle handle;
> - unsigned int func;
> const guid_t *guid;
> - int rc, i;
> + int func, rc, i;
>
> if (cmd_rc)
> *cmd_rc = -EINVAL;
> - func = cmd;
> - if (cmd == ND_CMD_CALL) {
> - call_pkg = buf;
This breaks ND_CMD_CALL because now call_pkg is NULL for the rest of
this routine. v4 inbound, as well as a backlog item to add an
ND_CMD_CALL test-case to nfit_ctl_test(). The incremental fix is:
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 9c95b82e5e5d..71d03a4004fb 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -412,10 +412,10 @@ static bool payload_dumpable(struct nvdimm
*nvdimm, unsigned int func)
static int cmd_to_func(struct nfit_mem *nfit_mem, unsigned int cmd,
struct nd_cmd_pkg *call_pkg)
{
- if (cmd == ND_CMD_CALL) {
+ if (call_pkg) {
int i;
- if (call_pkg && nfit_mem->family != call_pkg->nd_family)
+ if (nfit_mem->family != call_pkg->nd_family)
return -ENOTTY;
for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
@@ -460,7 +460,9 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
*nd_desc, struct nvdimm *nvdimm,
if (!adev)
return -ENOTTY;
- func = cmd_to_func(nfit_mem, cmd, buf);
+ if (cmd == ND_CMD_CALL)
+ call_pkg = buf;
+ func = cmd_to_func(nfit_mem, cmd, call_pkg);
if (func < 0)
return func;
dimm_name = nvdimm_name(nvdimm);
