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

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

[Dan Williams]

Changes since v1 [1]:
* Include another patch make sure that function-number zero can be
  safely used as an invalid function number (Jeff)
* Add a comment clarifying why zero is an invalid function number (Jeff)
* Pass nfit_mem to cmd_to_func() (Jeff)
* Collect a Tested-by from Sujith
[1]: https://lists.01.org/pipermail/linux-nvdimm/2019-January/019435.html

---

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 |   53 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 14 deletions(-)

[PATCH v2 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>
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 790691d9a982..67aef1a793d5 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 v2 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
Cc: Jeff Moyer <jmoyer@redhat.com>
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>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c |   46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 67aef1a793d5..f770758f5063 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,7 @@ 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))
+	if (!test_bit(cmd, &cmd_mask) && !test_bit(func, &dsm_mask))
 		return -ENOTTY;
 
 	in_obj.type = ACPI_TYPE_PACKAGE;

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

[Jeff Moyer]

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

> Changes since v1 [1]:
> * Include another patch make sure that function-number zero can be
>   safely used as an invalid function number (Jeff)
> * Add a comment clarifying why zero is an invalid function number (Jeff)
> * Pass nfit_mem to cmd_to_func() (Jeff)
> * Collect a Tested-by from Sujith
> [1]: https://lists.01.org/pipermail/linux-nvdimm/2019-January/019435.html

For the series:

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Thanks, Dan!


>
> ---
>
> 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 |   53 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 14 deletions(-)

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

[Dan Williams]

On Tue, Jan 15, 2019 at 6:16 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > Changes since v1 [1]:
> > * Include another patch make sure that function-number zero can be
> >   safely used as an invalid function number (Jeff)
> > * Add a comment clarifying why zero is an invalid function number (Jeff)
> > * Pass nfit_mem to cmd_to_func() (Jeff)
> > * Collect a Tested-by from Sujith
> > [1]: https://lists.01.org/pipermail/linux-nvdimm/2019-January/019435.html
>
> For the series:
>
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
>
> Thanks, Dan!

Thanks, although I just realized one more change. The ND_CMD_CALL case
should zero out command after the function translation, otherwise
userspace can call functions that the kernel is blocking in the
dsm_mask.

Holler if this invalidates your "Reviewed-by".

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 87e02f281e51..d7747aceb7ab 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -463,6 +463,12 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
*nd_desc, struct nvdimm *nvdimm,
                func = cmd_to_func(nfit_mem, cmd, buf);
                if (func < 0)
                        return func;
+               /*
+                * In the ND_CMD_CALL case we're now dependent on 'func'
+                * being validated by the dimm's dsm_mask
+                */
+               if (cmd == ND_CMD_CALL)
+                       cmd = 0;
                dimm_name = nvdimm_name(nvdimm);
                cmd_name = nvdimm_cmd_name(cmd);
                cmd_mask = nvdimm_cmd_mask(nvdimm);
@@ -477,8 +483,10 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
*nd_desc, struct nvdimm *nvdimm,
                cmd_name = nvdimm_bus_cmd_name(cmd);
                cmd_mask = nd_desc->cmd_mask;
                dsm_mask = cmd_mask;
-               if (cmd == ND_CMD_CALL)
+               if (cmd == ND_CMD_CALL) {
                        dsm_mask = nd_desc->bus_dsm_mask;
+                       cmd = 0;
+               }
                desc = nd_cmd_bus_desc(cmd);
                guid = to_nfit_uuid(NFIT_DEV_BUS);
                handle = adev->handle;

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

[Jeff Moyer]

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

> On Tue, Jan 15, 2019 at 6:16 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > Changes since v1 [1]:
>> > * Include another patch make sure that function-number zero can be
>> >   safely used as an invalid function number (Jeff)
>> > * Add a comment clarifying why zero is an invalid function number (Jeff)
>> > * Pass nfit_mem to cmd_to_func() (Jeff)
>> > * Collect a Tested-by from Sujith
>> > [1]: https://lists.01.org/pipermail/linux-nvdimm/2019-January/019435.html
>>
>> For the series:
>>
>> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
>>
>> Thanks, Dan!
>
> Thanks, although I just realized one more change. The ND_CMD_CALL case
> should zero out command after the function translation, otherwise
> userspace can call functions that the kernel is blocking in the
> dsm_mask.
>
> Holler if this invalidates your "Reviewed-by".

AAAAAAAHHHHHHHHHH!!!!

:)

> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 87e02f281e51..d7747aceb7ab 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -463,6 +463,12 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
> *nd_desc, struct nvdimm *nvdimm,
>                 func = cmd_to_func(nfit_mem, cmd, buf);
>                 if (func < 0)
>                         return func;
> +               /*
> +                * In the ND_CMD_CALL case we're now dependent on 'func'
> +                * being validated by the dimm's dsm_mask
> +                */
> +               if (cmd == ND_CMD_CALL)
> +                       cmd = 0;
>                 dimm_name = nvdimm_name(nvdimm);
>                 cmd_name = nvdimm_cmd_name(cmd);
>                 cmd_mask = nvdimm_cmd_mask(nvdimm);
                dsm_mask = nfit_mem->dsm_mask;
                desc = nd_cmd_dimm_desc(cmd);

That sure doesn't look right.  Now cmd_name and desc will be wrong.

> @@ -477,8 +483,10 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
> *nd_desc, struct nvdimm *nvdimm,
>                 cmd_name = nvdimm_bus_cmd_name(cmd);
>                 cmd_mask = nd_desc->cmd_mask;
>                 dsm_mask = cmd_mask;
> -               if (cmd == ND_CMD_CALL)
> +               if (cmd == ND_CMD_CALL) {
>                         dsm_mask = nd_desc->bus_dsm_mask;
> +                       cmd = 0;
> +               }
>                 desc = nd_cmd_bus_desc(cmd);

And again here.

We could reorder the zeroing, or you could modify the check for a valid
comand/function.  Something like this?

	/*
         * 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;

Which way do you think is cleaner?

Cheers,
Jeff

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

[Dan Williams]

On Tue, Jan 15, 2019 at 12:39 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > On Tue, Jan 15, 2019 at 6:16 AM Jeff Moyer <jmoyer@redhat.com> wrote:
> >>
> >> Dan Williams <dan.j.williams@intel.com> writes:
> >>
> >> > Changes since v1 [1]:
> >> > * Include another patch make sure that function-number zero can be
> >> >   safely used as an invalid function number (Jeff)
> >> > * Add a comment clarifying why zero is an invalid function number (Jeff)
> >> > * Pass nfit_mem to cmd_to_func() (Jeff)
> >> > * Collect a Tested-by from Sujith
> >> > [1]: https://lists.01.org/pipermail/linux-nvdimm/2019-January/019435.html
> >>
> >> For the series:
> >>
> >> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
> >>
> >> Thanks, Dan!
> >
> > Thanks, although I just realized one more change. The ND_CMD_CALL case
> > should zero out command after the function translation, otherwise
> > userspace can call functions that the kernel is blocking in the
> > dsm_mask.
> >
> > Holler if this invalidates your "Reviewed-by".
>
> AAAAAAAHHHHHHHHHH!!!!
>
> :)
>
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index 87e02f281e51..d7747aceb7ab 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -463,6 +463,12 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
> > *nd_desc, struct nvdimm *nvdimm,
> >                 func = cmd_to_func(nfit_mem, cmd, buf);
> >                 if (func < 0)
> >                         return func;
> > +               /*
> > +                * In the ND_CMD_CALL case we're now dependent on 'func'
> > +                * being validated by the dimm's dsm_mask
> > +                */
> > +               if (cmd == ND_CMD_CALL)
> > +                       cmd = 0;
> >                 dimm_name = nvdimm_name(nvdimm);
> >                 cmd_name = nvdimm_cmd_name(cmd);
> >                 cmd_mask = nvdimm_cmd_mask(nvdimm);
>                 dsm_mask = nfit_mem->dsm_mask;
>                 desc = nd_cmd_dimm_desc(cmd);
>
> That sure doesn't look right.  Now cmd_name and desc will be wrong.

Ah, whoops, yes good catch. Guess this shows there is not good
ND_CMD_CALL coverage in the unit tests...

>
> > @@ -477,8 +483,10 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor
> > *nd_desc, struct nvdimm *nvdimm,
> >                 cmd_name = nvdimm_bus_cmd_name(cmd);
> >                 cmd_mask = nd_desc->cmd_mask;
> >                 dsm_mask = cmd_mask;
> > -               if (cmd == ND_CMD_CALL)
> > +               if (cmd == ND_CMD_CALL) {
> >                         dsm_mask = nd_desc->bus_dsm_mask;
> > +                       cmd = 0;
> > +               }
> >                 desc = nd_cmd_bus_desc(cmd);
>
> And again here.
>
> We could reorder the zeroing, or you could modify the check for a valid
> comand/function.  Something like this?
>
>         /*
>          * 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;
>
> Which way do you think is cleaner?

Modifying the check looks cleaner. Thanks for hollering!