Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann <jerry.hoemann@xxxxxxx> wrote:
> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >>
> >> ...
> >>
> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann <jerry.hoemann@xxxxxxx> wrote:
> >>> > +               if (cmd == ND_CMD_CALL)
> >>> > +                       dsm_mask = nd_desc->bus_dsm_mask;
> >>> >                 desc = nd_cmd_bus_desc(cmd);
> >>> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >>> >                 handle = adev->handle;
> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >>> >         struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
> >>> >         const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >>> >         struct acpi_device *adev;
> >>> > +       unsigned long dsm_mask;
> >>> >         int i;
> >>> >
> >>> >         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >>> >                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >>> >                         set_bit(i, &nd_desc->cmd_mask);
> >>> >         set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
> >>> > +
> >>> > +       dsm_mask = 0x3bf;
> >>>
> >>> I went ahead and fixed this up to use dsm_mask defined like this:
> >>>
> >>> +       dsm_mask =
> >>> +               (1 << ND_CMD_ARS_CAP) |
> >>> +               (1 << ND_CMD_ARS_START) |
> >>> +               (1 << ND_CMD_ARS_STATUS) |
> >>> +               (1 << ND_CMD_CLEAR_ERROR) |
> >>> +               (1 << NFIT_CMD_TRANSLATE_SPA) |
> >>> +               (1 << NFIT_CMD_ARS_INJECT_SET) |
> >>> +               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> >>> +               (1 << NFIT_CMD_ARS_INJECT_GET);
> >>>
> >>> This drops function number 0 which userspace has no need to call.
> >>
> >> Actually I like to call function 0.  Its an excellent test when
> >> modifying the code path as its a no side effects function whose output
> >> is known in advance and instantly recognizable.  I also use it when
> >> testing new firmware.
> >>
> >> What is the downside to allowing it?  What bad things happen?
> >
> > It allows implementations to bypass the standardization process and
> > ship new root DSMs. It's always possible to patch the kernel locally
> > for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment 
essentially says to disregard this comment?

> 
> Actually, just the discovery portion does not lead to this leak, but
> it's redundant when we have the 'dsm_mask' sysfs attribute.

No.  The generation of the mask in sysfs is not done by
executing the code in acpi_nfit_ctl.  One of the reasons I call
function 0 to test changes I am making to the ioctl path itself.
The sysfs has nothing to do with that path and cannot be used
to serve this purpose.

And since the content of sysfs has been edited it also can not be
used as a basic test of firmware.

What is the downside to allowing the calling of function 0?



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------