* [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
@ 2019-01-15 1:57 Dan Williams
2019-01-15 1:57 ` [PATCH v2 1/2] acpi/nfit: Block function zero DSMs Dan Williams
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Dan Williams @ 2019-01-15 1:57 UTC (permalink / raw)
To: linux-nvdimm
Cc: stable, stuart hayes, Sujith Pandel, Jeff Moyer, Vishal Verma,
linux-kernel, vishal.l.verma
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(-)
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] acpi/nfit: Block function zero DSMs
2019-01-15 1:57 [PATCH v2 0/2] acpi/nfit: Fix command-supported detection Dan Williams
@ 2019-01-15 1:57 ` Dan Williams
2019-01-15 1:57 ` [PATCH v2 2/2] acpi/nfit: Fix command-supported detection Dan Williams
2019-01-15 14:16 ` [PATCH v2 0/2] " Jeff Moyer
2 siblings, 0 replies; 7+ messages in thread
From: Dan Williams @ 2019-01-15 1:57 UTC (permalink / raw)
To: linux-nvdimm
Cc: stable, stuart hayes, Jeff Moyer, vishal.l.verma, linux-kernel,
vishal.l.verma
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,
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] acpi/nfit: Fix command-supported detection
2019-01-15 1:57 [PATCH v2 0/2] acpi/nfit: Fix command-supported detection Dan Williams
2019-01-15 1:57 ` [PATCH v2 1/2] acpi/nfit: Block function zero DSMs Dan Williams
@ 2019-01-15 1:57 ` Dan Williams
2019-01-15 14:16 ` [PATCH v2 0/2] " Jeff Moyer
2 siblings, 0 replies; 7+ messages in thread
From: Dan Williams @ 2019-01-15 1:57 UTC (permalink / raw)
To: linux-nvdimm
Cc: stable, Jeff Moyer, Sujith Pandel, Sujith Pandel, Vishal Verma,
linux-kernel, vishal.l.verma
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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
2019-01-15 1:57 [PATCH v2 0/2] acpi/nfit: Fix command-supported detection Dan Williams
2019-01-15 1:57 ` [PATCH v2 1/2] acpi/nfit: Block function zero DSMs Dan Williams
2019-01-15 1:57 ` [PATCH v2 2/2] acpi/nfit: Fix command-supported detection Dan Williams
@ 2019-01-15 14:16 ` Jeff Moyer
2019-01-15 16:48 ` Dan Williams
2 siblings, 1 reply; 7+ messages in thread
From: Jeff Moyer @ 2019-01-15 14:16 UTC (permalink / raw)
To: Dan Williams
Cc: linux-nvdimm, stable, stuart hayes, Sujith Pandel, Vishal Verma,
linux-kernel
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(-)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
2019-01-15 14:16 ` [PATCH v2 0/2] " Jeff Moyer
@ 2019-01-15 16:48 ` Dan Williams
2019-01-15 20:39 ` Jeff Moyer
0 siblings, 1 reply; 7+ messages in thread
From: Dan Williams @ 2019-01-15 16:48 UTC (permalink / raw)
To: Jeff Moyer
Cc: linux-nvdimm, stable, stuart hayes, Sujith Pandel, Vishal Verma,
Linux Kernel Mailing List
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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
2019-01-15 16:48 ` Dan Williams
@ 2019-01-15 20:39 ` Jeff Moyer
2019-01-15 21:04 ` Dan Williams
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Moyer @ 2019-01-15 20:39 UTC (permalink / raw)
To: Dan Williams
Cc: linux-nvdimm, stable, stuart hayes, Sujith Pandel, Vishal Verma,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
2019-01-15 20:39 ` Jeff Moyer
@ 2019-01-15 21:04 ` Dan Williams
0 siblings, 0 replies; 7+ messages in thread
From: Dan Williams @ 2019-01-15 21:04 UTC (permalink / raw)
To: Jeff Moyer
Cc: linux-nvdimm, stable, stuart hayes, Sujith Pandel, Vishal Verma,
Linux Kernel Mailing List
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-15 21:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-15 1:57 [PATCH v2 0/2] acpi/nfit: Fix command-supported detection Dan Williams
2019-01-15 1:57 ` [PATCH v2 1/2] acpi/nfit: Block function zero DSMs Dan Williams
2019-01-15 1:57 ` [PATCH v2 2/2] acpi/nfit: Fix command-supported detection Dan Williams
2019-01-15 14:16 ` [PATCH v2 0/2] " Jeff Moyer
2019-01-15 16:48 ` Dan Williams
2019-01-15 20:39 ` Jeff Moyer
2019-01-15 21:04 ` Dan Williams
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).