nvdimm: Add an IOCTL pass thru for DSM calls

The ACPI spec speicifies that arguments "Revision ID" and "Function Index" to a _DSM are type "Integer." Type Integers are 64 bit quantities.

The function evaluate_dsm specifies these types as simple "int" which are 32 bits. Correct type passed to acpi_evaluate_dsm and its callers and derived callers to pass correct type.

acpi_check_dsm and acpi_evaluate_dsm_typed had similar issue and were corrected as well.

Signed-off-by Jerry Hoemann <[email protected]>

---
 drivers/acpi/utils.c    | 4 ++--
 include/acpi/acpi_bus.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

drivers/acpi/utils.c +2 -2

index 475c907..049cba4 100644
@@ -628,7 +628,7 @@ acpi_status acpi_evaluate_lck(acpi_handle handle, int lock)
  * some old BIOSes do expect a buffer or an integer etc.
  */
 union acpi_object *
-acpi_evaluate_dsm(acpi_handle handle, const u8 *uuid, int rev, int func,
+acpi_evaluate_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
 		  union acpi_object *argv4)
 {
 	acpi_status ret;

@@ -677,7 +677,7 @@ EXPORT_SYMBOL(acpi_evaluate_dsm);
  * functions. Currently only support 64 functions at maximum, should be
  * enough for now.
  */
-bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs)
+bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs)
 {
 	int i;
 	u64 mask = 0;

include/acpi/acpi_bus.h +3 -3

index ad0a5ff..8e6abcf 100644
@@ -61,12 +61,12 @@ bool acpi_ata_match(acpi_handle handle);
 bool acpi_bay_match(acpi_handle handle);
 bool acpi_dock_match(acpi_handle handle);
 
-bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs);
+bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs);
 union acpi_object *acpi_evaluate_dsm(acpi_handle handle, const u8 *uuid,
-			int rev, int func, union acpi_object *argv4);
+			u64 rev, u64 func, union acpi_object *argv4);
 
 static inline union acpi_object *
-acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, int rev, int func,
+acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
 			union acpi_object *argv4, acpi_object_type type)
 {
 	union acpi_object *obj;

The NVDIMM code in the kernel supports an IOCTL interface to user space based upon the Intel Example DSM:

http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf

This interface cannot be used by other NVDIMM DSMs that support incompatible functions.

This patch set adds a generic "passthru" IOCTL interface which is not tied to a particular DSM.

A new _IOC_NR ND_CMD_CALL_DSM == "10" is added for the pass thru call.

The new data structure nd_cmd_dsmcall_pkg serves as a wrapper for the passthru calls. This wrapper supplies the data that the kernel needs to make the _DSM call.

Unlike the definitions of the _DSM functions themselves, the nd_cmd_dsmcall_pkg provides the calling information (input/output sizes) in an uniform manner making the kernel marshaling of the arguments straight forward.

This shifts the marshaling burden from the kernel to the user space application while still permitting the kernel to internally call _DSM functions.

The kernel functions __nd_ioctl and acpi_nfit_ctl were modified to accomodate ND_CMD_CALL_DSM.

Changes in version 5:
---------------------
0. Fixed submit comment for drivers/acpi/utils.c.

Changes in version 4:
---------------------
0. Added patch to correct parameter type passed to acpi_evaluate_dsm ACPI defines arguments rev and fun as 64 bit quanties and the ioctl exports to user face rev and func. We want those to match the ACPI spec.

Also modified acpi_evaluate_dsm_typed and acpi_check dsm which had similar issue.

1. nd_cmd_dsmcall_pkg rearange a reserve and rounded up total size to 16 byte boundary.

2. Created stand alone patch for the pre-existing security issue related to "read only" IOCTL calls.

3. Added patch for increasing envelope size of IOCTL. Needed to be able to read in the wrapper to know remaining size to copy in.

Note: in_env, out_env are statics sized based upon this change.

4. Moved copyin code to table driven nd_cmd_desc

Note, the last 40 lines or so of acpi_nfit_ctl will not return _DSM data unless the size allocated in user space buffer equals out_obj->buffer.length.

The semantic we want in the pass thru case is to return as much of the _DSM data as the user space buffer would accomodate.

Hence, in acpi_nfit_ctl I have retained the line:

                memcpy(pkg->dsm_buf + pkg->h.dsm_in,
                        out_obj->buffer.pointer,
                        min(pkg->h.dsm_size, pkg->h.dsm_out));

and the early return from the function.

Changes in version 3:
---------------------
1. Changed name ND_CMD_PASSTHRU to ND_CMD_CALL_DSM.

2. Value of ND_CMD_CALL_DSM is 10, not 100.

3. Changed name of nd_passthru_pkg to nd_cmd_dsmcall_pkg.

4. Removed separate functions for handling ND_CMD_CALL_DSM. Moved functionality to __nd_ioctl and acpi_nfit_ctl proper. The resultant code looks very different from prior versions.

5. BUGFIX: __nd_ioctl: Change the if read_only switch to use _IOC_NR cmd (not ioctl_cmd) for better protection.

Do we want to make a stand alone patch for this issue?

Changes in version 2:
---------------------
1. Cleanup access mode check in nd_ioctl and nvdimm_ioctl.
2. Change name of ndn_pkg to nd_passthru_pkg
3. Adjust sizes in nd_passthru_pkg. DSM intergers are 64 bit.
4. No new ioctl type, instead tunnel into the existing number space.
5. Push down one function level where determine ioctl cmd type.
6. re-work diagnostic print/dump message in pass-thru functions.

Jerry Hoemann (6):
ACPI / util: Fix acpi_evaluate_dsm() argument type
nvdimm: Clean-up access mode check.
nvdimm: Add wrapper for IOCTL pass thru
nvdimm: Fix security issue with DSM IOCTL.
nvdimm: Increase max envelope size for IOCTL
nvdimm: Add IOCTL pass thru functions

On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann <[email protected]> wrote:

Quoted 7 lines

Jerry Hoemann (6):
ACPI / util: Fix acpi_evaluate_dsm() argument type
nvdimm: Clean-up access mode check.
nvdimm: Add wrapper for IOCTL pass thru
nvdimm: Fix security issue with DSM IOCTL.
nvdimm: Increase max envelope size for IOCTL
nvdimm: Add IOCTL pass thru functions

These look good to me.

I'll tag "nvdimm: Fix security issue with DSM IOCTL." for -stable.

Thanks Jerry!

Quoted 1 line · Dan Williams

On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann <[email protected]> wrote:

[..]

Quoted 10 lines

Quoted 7 lines

Jerry Hoemann (6):
ACPI / util: Fix acpi_evaluate_dsm() argument type
nvdimm: Clean-up access mode check.
nvdimm: Add wrapper for IOCTL pass thru
nvdimm: Fix security issue with DSM IOCTL.
nvdimm: Increase max envelope size for IOCTL
nvdimm: Add IOCTL pass thru functions

These look good to me.

Quoted 3 lines

I'll tag "nvdimm: Fix security issue with DSM IOCTL." for -stable.

Thanks Jerry!

I went to go write a test / support in ndctl for these and noticed a few things I want to address before merging.

1/ Advertise 'call_dsm' as a supported command alongside the others.

2/ Disallow potentially invalid calls to reach firmware. At a minimum the kernel needs to know the uuid in advance for any dsm it wants to send. I.e. check the 'dsm_fun_idx' against the dsm_mask. This is also important for making sure the kernel can manage exclusive access to the configuration data area if present (ND_CMD_{GET|SET}_CONFIG_DATA).

3/ This is minor, but it follows from 1/ that there may be some nvdimm bus implementations that do not implement 'call_dsm' support. 'nfit_test' is currently one of those buses and we need to check for that explicitly in nd_ioctl.

I have some patches in progress to address these.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Dan Williams

On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams <[email protected]>wrote:

Quoted 6 lines

2/ Disallow potentially invalid calls to reach firmware. At a minimum the kernel needs to know the uuid in advance for any dsm it wants to send. I.e. check the 'dsm_fun_idx' against the dsm_mask. This is also important for making sure the kernel can manage exclusive access to the configuration data area if present (ND_CMD_{GET|SET}_CONFIG_DATA).

Technically, the kernel doesn't need to know the uuid in advance as that is part of the bundle passed into the passthru.

Are you concerned about firmware mis-behaving when presented with a (UUID, Function_Index) that is not supported? (and really we should add Revision ID to that tuple.)

In a prior version of the patch not sent upstream, I did "discover" the uuid and set up the dsm_mask. However, this created a need to modify kernel each time uuid changes. Also, i don't think this is necessary as FW should be gracefully validating its input arguments. By not setting up/using dsm_mask in pass thru case, this can be tested.

I don't understand the exclusive access concern w/ config data. Could you please elaborate?