Re: [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
On Tue, Jan 15, 2019 at 12:39 PM Jeff Moyer wrote:
>
> Dan Williams writes:
>
> > On Tue, Jan 15, 2019 at 6:16 AM Jeff Moyer wrote:
> >>
> >> Dan Williams 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
> >>
> >> 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".
>
> AAAHH
>
> :)
>
> > 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, _mask))
> return -ENOTTY;
> else if (!test_bit(cmd, _mask))
> return -ENOTTY;
>
> Which way do you think is cleaner?
Modifying the check looks cleaner. Thanks for hollering!
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
Dan Williams writes:
> On Tue, Jan 15, 2019 at 6:16 AM Jeff Moyer wrote:
>>
>> Dan Williams 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
>>
>> 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".
AAAHH
:)
> 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, _mask))
return -ENOTTY;
else if (!test_bit(cmd, _mask))
return -ENOTTY;
Which way do you think is cleaner?
Cheers,
Jeff
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
On Tue, Jan 15, 2019 at 6:16 AM Jeff Moyer wrote:
>
> Dan Williams 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
>
> 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;
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v2 0/2] acpi/nfit: Fix command-supported detection
Dan Williams 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
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(-)
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
