[PATCH v3 0/2] acpi/nfit: Fix command-supported detection

[PATCH v3 0/2] acpi/nfit: Fix command-supported detection

[Dan Williams]

Changes since v2 [1]:
* Don't allow ND_CMD_CALL to bypass dsm_mask restrictions (Jeff)

[1]: https://lists.01.org/pipermail/linux-nvdimm/2019-January/019498.html

---

One last resend to make sure all the last bits of thrash have settled.

Quote patch2 changelog:

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.

---

Dan Williams (2):
      acpi/nfit: Block function zero DSMs
      acpi/nfit: Fix command-supported detection


 drivers/acpi/nfit/core.c |   59 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 14 deletions(-)

[PATCH v3 1/2] acpi/nfit: Block function zero DSMs

[Dan Williams]

In preparation for using function number 0 as an error value, prevent it
from being considered a valid function value by acpi_nfit_ctl().

Cc: <stable@vger.kernel.org>
Cc: stuart hayes <stuart.w.hayes@gmail.com>
Fixes: e02fb7264d8a ("nfit: add Microsoft NVDIMM DSM command set...")
Reported-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 5143e11e3b0f..73281b19d3dd 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1867,6 +1867,13 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 		return 0;
 	}
 
+	/*
+	 * Function 0 is the command interrogation function, don't
+	 * export it to potential userspace use, and enable it to be
+	 * used as an error value in acpi_nfit_ctl().
+	 */
+	dsm_mask &= ~1UL;
+
 	guid = to_nfit_uuid(nfit_mem->family);
 	for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
 		if (acpi_check_dsm(adev_dimm->handle, guid,

[PATCH v3 2/2] acpi/nfit: Fix command-supported detection

[Dan Williams]

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@vger.kernel.org>
Link: https://github.com/pmem/ndctl/issues/78
Reported-by: Sujith Pandel <sujith_pandel@dell.com>
Tested-by: Sujith Pandel <sujith_pandel@dell.com>
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 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;
-		func = call_pkg->nd_command;
-
-		for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
-			if (call_pkg->nd_reserved2[i])
-				return -EINVAL;
-	}
 
 	if (nvdimm) {
 		struct acpi_device *adev = nfit_mem->adev;
 
 		if (!adev)
 			return -ENOTTY;
-		if (call_pkg && nfit_mem->family != call_pkg->nd_family)
-			return -ENOTTY;
 
+		func = cmd_to_func(nfit_mem, cmd, buf);
+		if (func < 0)
+			return func;
 		dimm_name = nvdimm_name(nvdimm);
 		cmd_name = nvdimm_cmd_name(cmd);
 		cmd_mask = nvdimm_cmd_mask(nvdimm);
@@ -456,6 +473,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 	} else {
 		struct acpi_device *adev = to_acpi_dev(acpi_desc);
 
+		func = cmd;
 		cmd_name = nvdimm_bus_cmd_name(cmd);
 		cmd_mask = nd_desc->cmd_mask;
 		dsm_mask = cmd_mask;
@@ -470,7 +488,13 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 	if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
 		return -ENOTTY;
 
-	if (!test_bit(cmd, &cmd_mask) || !test_bit(func, &dsm_mask))
+	/*
+	 * Check for a valid command.  For ND_CMD_CALL, we also have to
+	 * make sure that the DSM function is supported.
+	 */
+	if (cmd == ND_CMD_CALL && !test_bit(func, &dsm_mask))
+		return -ENOTTY;
+	else if (!test_bit(cmd, &cmd_mask))
 		return -ENOTTY;
 
 	in_obj.type = ACPI_TYPE_PACKAGE;

Re: [PATCH v3 0/2] acpi/nfit: Fix command-supported detection

[Jeff Moyer]

Dan Williams <dan.j.williams@intel.com> writes:

> Changes since v2 [1]:
> * Don't allow ND_CMD_CALL to bypass dsm_mask restrictions (Jeff)
>
> [1]: https://lists.01.org/pipermail/linux-nvdimm/2019-January/019498.html
>
> ---
>
> One last resend to make sure all the last bits of thrash have settled.

LGTM.

Thanks!
Jeff

Re: [PATCH v3 2/2] acpi/nfit: Fix command-supported detection

[Dan Williams]

On Tue, Jan 15, 2019 at 2:46 PM Dan Williams <dan.j.williams@intel.com> 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@vger.kernel.org>
> Link: https://github.com/pmem/ndctl/issues/78
> Reported-by: Sujith Pandel <sujith_pandel@dell.com>
> Tested-by: Sujith Pandel <sujith_pandel@dell.com>
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  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);