[PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

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

 drivers/acpi/nfit.c        | 52 ++++++++++++++++++++++++++---------
 drivers/acpi/utils.c       |  4 +--
 drivers/nvdimm/bus.c       | 67 +++++++++++++++++++++++++++++++++++++---------
 include/acpi/acpi_bus.h    |  6 ++---
 include/linux/libnvdimm.h  |  2 +-
 include/uapi/linux/ndctl.h | 19 +++++++++++++
 6 files changed, 118 insertions(+), 32 deletions(-)

-- 
1.7.11.3

[PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

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

 drivers/acpi/nfit.c        | 52 ++++++++++++++++++++++++++---------
 drivers/acpi/utils.c       |  4 +--
 drivers/nvdimm/bus.c       | 67 +++++++++++++++++++++++++++++++++++++---------
 include/acpi/acpi_bus.h    |  6 ++---
 include/linux/libnvdimm.h  |  2 +-
 include/uapi/linux/ndctl.h | 19 +++++++++++++
 6 files changed, 118 insertions(+), 32 deletions(-)

-- 
1.7.11.3

[PATCH v5 1/6] ACPI / util: Fix acpi_evaluate_dsm() argument type

[Jerry Hoemann]

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 <jerry.hoemann@hpe.com>
---
 drivers/acpi/utils.c    | 4 ++--
 include/acpi/acpi_bus.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 475c907..049cba4 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -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;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ad0a5ff..8e6abcf 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -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;
-- 
1.7.11.3

[PATCH v5 1/6] ACPI / util: Fix acpi_evaluate_dsm() argument type

[Jerry Hoemann]

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 <jerry.hoemann@hpe.com>
---
 drivers/acpi/utils.c    | 4 ++--
 include/acpi/acpi_bus.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 475c907..049cba4 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -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;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ad0a5ff..8e6abcf 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -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;
-- 
1.7.11.3

[PATCH v5 2/6] nvdimm: Clean-up access mode check.

[Jerry Hoemann]

Change nd_ioctl and nvdimm_ioctl access mode check to use O_RDONLY.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/nvdimm/bus.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..1c81203 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -602,14 +602,14 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long id = (long) file->private_data;
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		if (nvdimm_bus->id == id) {
-			rc = __nd_ioctl(nvdimm_bus, NULL, read_only, cmd, arg);
+			rc = __nd_ioctl(nvdimm_bus, NULL, ro, cmd, arg);
 			break;
 		}
 	}
@@ -633,10 +633,10 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		struct device *dev = device_find_child(&nvdimm_bus->dev,
@@ -647,7 +647,7 @@ static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			continue;
 
 		nvdimm = to_nvdimm(dev);
-		rc = __nd_ioctl(nvdimm_bus, nvdimm, read_only, cmd, arg);
+		rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
 		put_device(dev);
 		break;
 	}
-- 
1.7.11.3

[PATCH v5 2/6] nvdimm: Clean-up access mode check.

[Jerry Hoemann]

Change nd_ioctl and nvdimm_ioctl access mode check to use O_RDONLY.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/nvdimm/bus.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..1c81203 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -602,14 +602,14 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long id = (long) file->private_data;
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		if (nvdimm_bus->id == id) {
-			rc = __nd_ioctl(nvdimm_bus, NULL, read_only, cmd, arg);
+			rc = __nd_ioctl(nvdimm_bus, NULL, ro, cmd, arg);
 			break;
 		}
 	}
@@ -633,10 +633,10 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		struct device *dev = device_find_child(&nvdimm_bus->dev,
@@ -647,7 +647,7 @@ static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			continue;
 
 		nvdimm = to_nvdimm(dev);
-		rc = __nd_ioctl(nvdimm_bus, nvdimm, read_only, cmd, arg);
+		rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
 		put_device(dev);
 		break;
 	}
-- 
1.7.11.3

[PATCH v5 3/6] nvdimm: Add wrapper for IOCTL pass thru

[Jerry Hoemann]

Add struct nd_passthru_pkg which serves as a warapper for
the data being passed via a pass thru to a NVDIMM DSM.
This wrapper specifies the extra information in a uniform
manner allowing the kenrel to call a DSM without knowing
specifics of the DSM.

Add dsm_call command to nvdimm_bus_cmd_name and nvdimm_cmd_name.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/uapi/linux/ndctl.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 5b4a4be..6823af3 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -109,6 +109,7 @@ enum {
 	ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7,
 	ND_CMD_VENDOR_EFFECT_LOG = 8,
 	ND_CMD_VENDOR = 9,
+	ND_CMD_CALL_DSM = 10,
 };
 
 enum {
@@ -122,6 +123,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 		[ND_CMD_ARS_CAP] = "ars_cap",
 		[ND_CMD_ARS_START] = "ars_start",
 		[ND_CMD_ARS_STATUS] = "ars_status",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -141,6 +143,7 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
 		[ND_CMD_VENDOR_EFFECT_LOG_SIZE] = "effect_size",
 		[ND_CMD_VENDOR_EFFECT_LOG] = "effect_log",
 		[ND_CMD_VENDOR] = "vendor",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -204,4 +207,20 @@ enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
 };
+
+
+struct nd_cmd_dsmcall_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u64	reserved1;		/* reserved should be zero */
+		__u64	dsm_rev;		/* revision of dsm call  */
+		__u64	dsm_fun_idx;		/* DSM function id	 */
+		__u32	dsm_in;			/* size of _DSM input	 */
+		__u32	dsm_out;		/* size of user buffer	 */
+		__u32	reserved2[23];		/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char dsm_buf[];		/* Contents of DSM call	 */
+};
+
 #endif /* __NDCTL_H__ */
-- 
1.7.11.3

[PATCH v5 3/6] nvdimm: Add wrapper for IOCTL pass thru

[Jerry Hoemann]

Add struct nd_passthru_pkg which serves as a warapper for
the data being passed via a pass thru to a NVDIMM DSM.
This wrapper specifies the extra information in a uniform
manner allowing the kenrel to call a DSM without knowing
specifics of the DSM.

Add dsm_call command to nvdimm_bus_cmd_name and nvdimm_cmd_name.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/uapi/linux/ndctl.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 5b4a4be..6823af3 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -109,6 +109,7 @@ enum {
 	ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7,
 	ND_CMD_VENDOR_EFFECT_LOG = 8,
 	ND_CMD_VENDOR = 9,
+	ND_CMD_CALL_DSM = 10,
 };
 
 enum {
@@ -122,6 +123,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 		[ND_CMD_ARS_CAP] = "ars_cap",
 		[ND_CMD_ARS_START] = "ars_start",
 		[ND_CMD_ARS_STATUS] = "ars_status",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -141,6 +143,7 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
 		[ND_CMD_VENDOR_EFFECT_LOG_SIZE] = "effect_size",
 		[ND_CMD_VENDOR_EFFECT_LOG] = "effect_log",
 		[ND_CMD_VENDOR] = "vendor",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -204,4 +207,20 @@ enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
 };
+
+
+struct nd_cmd_dsmcall_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u64	reserved1;		/* reserved should be zero */
+		__u64	dsm_rev;		/* revision of dsm call  */
+		__u64	dsm_fun_idx;		/* DSM function id	 */
+		__u32	dsm_in;			/* size of _DSM input	 */
+		__u32	dsm_out;		/* size of user buffer	 */
+		__u32	reserved2[23];		/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char dsm_buf[];		/* Contents of DSM call	 */
+};
+
 #endif /* __NDCTL_H__ */
-- 
1.7.11.3

[PATCH v5 4/6] nvdimm: Fix security issue with DSM IOCTL.

[Jerry Hoemann]

Code attempts to prevent certain IOCTL DSM from being called
when device is opened read only.  This security feature can
be trivially overcome by changing the size portion of the
ioctl_command which isn't used.

Check only the _IOC_NR (i.e. the command).

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/nvdimm/bus.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 1c81203..87fe545 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -513,10 +513,10 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 
 	/* fail write commands (when read-only) */
 	if (read_only)
-		switch (ioctl_cmd) {
-		case ND_IOCTL_VENDOR:
-		case ND_IOCTL_SET_CONFIG_DATA:
-		case ND_IOCTL_ARS_START:
+		switch (cmd) {
+		case ND_CMD_VENDOR:
+		case ND_CMD_SET_CONFIG_DATA:
+		case ND_CMD_ARS_START:
 			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
-- 
1.7.11.3

[PATCH v5 4/6] nvdimm: Fix security issue with DSM IOCTL.

[Jerry Hoemann]

Code attempts to prevent certain IOCTL DSM from being called
when device is opened read only.  This security feature can
be trivially overcome by changing the size portion of the
ioctl_command which isn't used.

Check only the _IOC_NR (i.e. the command).

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/nvdimm/bus.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 1c81203..87fe545 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -513,10 +513,10 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 
 	/* fail write commands (when read-only) */
 	if (read_only)
-		switch (ioctl_cmd) {
-		case ND_IOCTL_VENDOR:
-		case ND_IOCTL_SET_CONFIG_DATA:
-		case ND_IOCTL_ARS_START:
+		switch (cmd) {
+		case ND_CMD_VENDOR:
+		case ND_CMD_SET_CONFIG_DATA:
+		case ND_CMD_ARS_START:
 			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
-- 
1.7.11.3

[PATCH v5 5/6] nvdimm: Increase max envelope size for IOCTL

[Jerry Hoemann]

In __nd_ioctl must first read in the fixed sized portion of an ioctl
so that it can then determine the size of the variable part.

Prepare for ND_CMD_CALL_DSM calls which have larger fixed portion
wrapper.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/linux/libnvdimm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 3f021dc..b0a2f60 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -27,7 +27,7 @@ enum {
 	/* need to set a limit somewhere, but yes, this is likely overkill */
 	ND_IOCTL_MAX_BUFLEN = SZ_4M,
 	ND_CMD_MAX_ELEM = 4,
-	ND_CMD_MAX_ENVELOPE = 16,
+	ND_CMD_MAX_ENVELOPE = 256,
 	ND_CMD_ARS_STATUS_MAX = SZ_4K,
 	ND_MAX_MAPPINGS = 32,
 
-- 
1.7.11.3

[PATCH v5 5/6] nvdimm: Increase max envelope size for IOCTL

[Jerry Hoemann]

In __nd_ioctl must first read in the fixed sized portion of an ioctl
so that it can then determine the size of the variable part.

Prepare for ND_CMD_CALL_DSM calls which have larger fixed portion
wrapper.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/linux/libnvdimm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 3f021dc..b0a2f60 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -27,7 +27,7 @@ enum {
 	/* need to set a limit somewhere, but yes, this is likely overkill */
 	ND_IOCTL_MAX_BUFLEN = SZ_4M,
 	ND_CMD_MAX_ELEM = 4,
-	ND_CMD_MAX_ENVELOPE = 16,
+	ND_CMD_MAX_ENVELOPE = 256,
 	ND_CMD_ARS_STATUS_MAX = SZ_4K,
 	ND_MAX_MAPPINGS = 32,
 
-- 
1.7.11.3

[PATCH v5 6/6] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

Add ioctl command ND_CMD_CALL_DSM to acpi_nfit_ctl and __nd_ioctl which
allow kernel to call a nvdimm's _DSM as a passthru without using the
marshaling code of the nd_cmd_desc.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.c  | 52 +++++++++++++++++++++++++++++++++++++++-------------
 drivers/nvdimm/bus.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 83 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index aa45d48..015fc8e 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -85,6 +85,10 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	const u8 *uuid;
 	u32 offset;
 	int rc, i;
+	__u64 rev = 1, func = cmd;
+
+	struct nd_cmd_dsmcall_pkg *pkg = buf;
+	int dsm_call = (cmd == ND_CMD_CALL_DSM);
 
 	if (nvdimm) {
 		struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
@@ -108,6 +112,8 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 		handle = adev->handle;
 		dimm_name = "bus";
 	}
+	if (dsm_call)
+		dsm_mask = ~0UL;
 
 	if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
 		return -ENOTTY;
@@ -127,15 +133,25 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 		in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc,
 				i, buf);
 
+	if (dsm_call) {
+		/* must skip over package wrapper */
+		in_buf.buffer.pointer = (void *) &pkg->dsm_buf;
+		in_buf.buffer.length = pkg->h.dsm_in;
+		/* for pass thru must use value sent in from user space. */
+		uuid = pkg->h.dsm_uuid;
+		rev  = pkg->h.dsm_rev;
+		func = pkg->h.dsm_fun_idx;
+	}
+
 	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-		dev_dbg(dev, "%s:%s cmd: %s input length: %d\n", __func__,
-				dimm_name, cmd_name, in_buf.buffer.length);
-		print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4,
-				4, in_buf.buffer.pointer, min_t(u32, 128,
-					in_buf.buffer.length), true);
+		dev_dbg(dev, "%s:%s cmd: %d: %llu input length: %d\n", __func__,
+				dimm_name, cmd, func, in_buf.buffer.length);
+		print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
+			in_buf.buffer.pointer,
+			min_t(u32, 256, in_buf.buffer.length), true);
 	}
 
-	out_obj = acpi_evaluate_dsm(handle, uuid, 1, cmd, &in_obj);
+	out_obj = acpi_evaluate_dsm(handle, uuid, rev, func, &in_obj);
 	if (!out_obj) {
 		dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
 				cmd_name);
@@ -143,18 +159,28 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	}
 
 	if (out_obj->package.type != ACPI_TYPE_BUFFER) {
-		dev_dbg(dev, "%s:%s unexpected output object type cmd: %s type: %d\n",
-				__func__, dimm_name, cmd_name, out_obj->type);
+		dev_dbg(dev, "%s:%s unexpected output object type cmd: %s %llu, type: %d\n",
+			__func__, dimm_name, cmd_name, func, out_obj->type);
 		rc = -EINVAL;
 		goto out;
 	}
 
 	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-		dev_dbg(dev, "%s:%s cmd: %s output length: %d\n", __func__,
-				dimm_name, cmd_name, out_obj->buffer.length);
-		print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4,
-				4, out_obj->buffer.pointer, min_t(u32, 128,
-					out_obj->buffer.length), true);
+		dev_dbg(dev, "%s:%s cmd %d: %llu output length %d\n", __func__,
+				dimm_name, cmd, func, out_obj->buffer.length);
+		print_hex_dump_debug("nvdimm out ", DUMP_PREFIX_OFFSET, 4, 4,
+			out_obj->buffer.pointer,
+			min_t(u32, 256, out_obj->buffer.length), true);
+	}
+
+	if (dsm_call) {
+		pkg->h.dsm_size = out_obj->buffer.length;
+		memcpy(pkg->dsm_buf + pkg->h.dsm_in,
+			out_obj->buffer.pointer,
+			min(pkg->h.dsm_size, pkg->h.dsm_out));
+
+		ACPI_FREE(out_obj);
+		return 0;
 	}
 
 	for (i = 0, offset = 0; i < desc->out_num; i++) {
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 87fe545..8d3a64b 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -367,6 +367,12 @@ static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = {
 		.out_num = 3,
 		.out_sizes = { 4, 4, UINT_MAX, },
 	},
+	[ND_CMD_CALL_DSM] = {
+		.in_num = 2,
+		.in_sizes = {sizeof(struct nd_cmd_dsmcall_pkg), UINT_MAX, },
+		.out_num = 1,
+		.out_sizes = { UINT_MAX, },
+	},
 };
 
 const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd)
@@ -395,6 +401,12 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
 		.out_num = 2,
 		.out_sizes = { 4, UINT_MAX, },
 	},
+	[ND_CMD_CALL_DSM] = {
+		.in_num = 2,
+		.in_sizes = {sizeof(struct nd_cmd_dsmcall_pkg), UINT_MAX, },
+		.out_num = 1,
+		.out_sizes = { UINT_MAX, },
+	},
 };
 
 const struct nd_cmd_desc *nd_cmd_bus_desc(int cmd)
@@ -422,6 +434,10 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd,
 		struct nd_cmd_vendor_hdr *hdr = buf;
 
 		return hdr->in_length;
+	} else if (cmd == ND_CMD_CALL_DSM) {
+		struct nd_cmd_dsmcall_pkg *pkg = buf;
+
+		return pkg->h.dsm_in;
 	}
 
 	return UINT_MAX;
@@ -444,6 +460,12 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd,
 		return out_field[1];
 	else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 1)
 		return ND_CMD_ARS_STATUS_MAX;
+	else if (cmd == ND_CMD_CALL_DSM) {
+		struct nd_cmd_dsmcall_pkg *pkg =
+				(struct nd_cmd_dsmcall_pkg *) in_field;
+		return pkg->h.dsm_out;
+	}
+
 
 	return UINT_MAX;
 }
@@ -491,19 +513,24 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	void __user *p = (void __user *) arg;
 	struct device *dev = &nvdimm_bus->dev;
 	const char *cmd_name, *dimm_name;
-	unsigned long dsm_mask;
+	unsigned long dsm_mask = ~0UL;
 	void *buf;
 	int rc, i;
 
+	struct nd_cmd_dsmcall_pkg *pkg;
+	int dsm_call = (cmd == ND_CMD_CALL_DSM);
+
 	if (nvdimm) {
 		desc = nd_cmd_dimm_desc(cmd);
 		cmd_name = nvdimm_cmd_name(cmd);
-		dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0;
+		if (!dsm_call)
+			dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0;
 		dimm_name = dev_name(&nvdimm->dev);
 	} else {
 		desc = nd_cmd_bus_desc(cmd);
 		cmd_name = nvdimm_bus_cmd_name(cmd);
-		dsm_mask = nd_desc->dsm_mask;
+		if (!dsm_call)
+			dsm_mask = nd_desc->dsm_mask;
 		dimm_name = "bus";
 	}
 
@@ -517,6 +544,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		case ND_CMD_VENDOR:
 		case ND_CMD_SET_CONFIG_DATA:
 		case ND_CMD_ARS_START:
+		case ND_CMD_CALL_DSM:
 			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
@@ -544,6 +572,19 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		in_len += in_size;
 	}
 
+	if (dsm_call) {
+		pkg = (struct nd_cmd_dsmcall_pkg *) in_env;
+
+		dev_dbg(dev, "%s:%s rev: %llu, idx: %llu, in: %zu, out: %zu, len %zu\n",
+			__func__, dimm_name,
+			pkg->h.dsm_rev, pkg->h.dsm_fun_idx,
+			in_len, out_len, buf_len);
+
+		for (i = 0; i < ARRAY_SIZE(pkg->h.reserved2); i++)
+			if (pkg->h.reserved2[i])
+				return -EINVAL;
+	}
+
 	/* process an output envelope */
 	for (i = 0; i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
-- 
1.7.11.3

[PATCH v5 6/6] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

Add ioctl command ND_CMD_CALL_DSM to acpi_nfit_ctl and __nd_ioctl which
allow kernel to call a nvdimm's _DSM as a passthru without using the
marshaling code of the nd_cmd_desc.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.c  | 52 +++++++++++++++++++++++++++++++++++++++-------------
 drivers/nvdimm/bus.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 83 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index aa45d48..015fc8e 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -85,6 +85,10 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	const u8 *uuid;
 	u32 offset;
 	int rc, i;
+	__u64 rev = 1, func = cmd;
+
+	struct nd_cmd_dsmcall_pkg *pkg = buf;
+	int dsm_call = (cmd == ND_CMD_CALL_DSM);
 
 	if (nvdimm) {
 		struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
@@ -108,6 +112,8 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 		handle = adev->handle;
 		dimm_name = "bus";
 	}
+	if (dsm_call)
+		dsm_mask = ~0UL;
 
 	if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
 		return -ENOTTY;
@@ -127,15 +133,25 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 		in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc,
 				i, buf);
 
+	if (dsm_call) {
+		/* must skip over package wrapper */
+		in_buf.buffer.pointer = (void *) &pkg->dsm_buf;
+		in_buf.buffer.length = pkg->h.dsm_in;
+		/* for pass thru must use value sent in from user space. */
+		uuid = pkg->h.dsm_uuid;
+		rev  = pkg->h.dsm_rev;
+		func = pkg->h.dsm_fun_idx;
+	}
+
 	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-		dev_dbg(dev, "%s:%s cmd: %s input length: %d\n", __func__,
-				dimm_name, cmd_name, in_buf.buffer.length);
-		print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4,
-				4, in_buf.buffer.pointer, min_t(u32, 128,
-					in_buf.buffer.length), true);
+		dev_dbg(dev, "%s:%s cmd: %d: %llu input length: %d\n", __func__,
+				dimm_name, cmd, func, in_buf.buffer.length);
+		print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
+			in_buf.buffer.pointer,
+			min_t(u32, 256, in_buf.buffer.length), true);
 	}
 
-	out_obj = acpi_evaluate_dsm(handle, uuid, 1, cmd, &in_obj);
+	out_obj = acpi_evaluate_dsm(handle, uuid, rev, func, &in_obj);
 	if (!out_obj) {
 		dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
 				cmd_name);
@@ -143,18 +159,28 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	}
 
 	if (out_obj->package.type != ACPI_TYPE_BUFFER) {
-		dev_dbg(dev, "%s:%s unexpected output object type cmd: %s type: %d\n",
-				__func__, dimm_name, cmd_name, out_obj->type);
+		dev_dbg(dev, "%s:%s unexpected output object type cmd: %s %llu, type: %d\n",
+			__func__, dimm_name, cmd_name, func, out_obj->type);
 		rc = -EINVAL;
 		goto out;
 	}
 
 	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-		dev_dbg(dev, "%s:%s cmd: %s output length: %d\n", __func__,
-				dimm_name, cmd_name, out_obj->buffer.length);
-		print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4,
-				4, out_obj->buffer.pointer, min_t(u32, 128,
-					out_obj->buffer.length), true);
+		dev_dbg(dev, "%s:%s cmd %d: %llu output length %d\n", __func__,
+				dimm_name, cmd, func, out_obj->buffer.length);
+		print_hex_dump_debug("nvdimm out ", DUMP_PREFIX_OFFSET, 4, 4,
+			out_obj->buffer.pointer,
+			min_t(u32, 256, out_obj->buffer.length), true);
+	}
+
+	if (dsm_call) {
+		pkg->h.dsm_size = out_obj->buffer.length;
+		memcpy(pkg->dsm_buf + pkg->h.dsm_in,
+			out_obj->buffer.pointer,
+			min(pkg->h.dsm_size, pkg->h.dsm_out));
+
+		ACPI_FREE(out_obj);
+		return 0;
 	}
 
 	for (i = 0, offset = 0; i < desc->out_num; i++) {
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 87fe545..8d3a64b 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -367,6 +367,12 @@ static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = {
 		.out_num = 3,
 		.out_sizes = { 4, 4, UINT_MAX, },
 	},
+	[ND_CMD_CALL_DSM] = {
+		.in_num = 2,
+		.in_sizes = {sizeof(struct nd_cmd_dsmcall_pkg), UINT_MAX, },
+		.out_num = 1,
+		.out_sizes = { UINT_MAX, },
+	},
 };
 
 const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd)
@@ -395,6 +401,12 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
 		.out_num = 2,
 		.out_sizes = { 4, UINT_MAX, },
 	},
+	[ND_CMD_CALL_DSM] = {
+		.in_num = 2,
+		.in_sizes = {sizeof(struct nd_cmd_dsmcall_pkg), UINT_MAX, },
+		.out_num = 1,
+		.out_sizes = { UINT_MAX, },
+	},
 };
 
 const struct nd_cmd_desc *nd_cmd_bus_desc(int cmd)
@@ -422,6 +434,10 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd,
 		struct nd_cmd_vendor_hdr *hdr = buf;
 
 		return hdr->in_length;
+	} else if (cmd == ND_CMD_CALL_DSM) {
+		struct nd_cmd_dsmcall_pkg *pkg = buf;
+
+		return pkg->h.dsm_in;
 	}
 
 	return UINT_MAX;
@@ -444,6 +460,12 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd,
 		return out_field[1];
 	else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 1)
 		return ND_CMD_ARS_STATUS_MAX;
+	else if (cmd == ND_CMD_CALL_DSM) {
+		struct nd_cmd_dsmcall_pkg *pkg =
+				(struct nd_cmd_dsmcall_pkg *) in_field;
+		return pkg->h.dsm_out;
+	}
+
 
 	return UINT_MAX;
 }
@@ -491,19 +513,24 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	void __user *p = (void __user *) arg;
 	struct device *dev = &nvdimm_bus->dev;
 	const char *cmd_name, *dimm_name;
-	unsigned long dsm_mask;
+	unsigned long dsm_mask = ~0UL;
 	void *buf;
 	int rc, i;
 
+	struct nd_cmd_dsmcall_pkg *pkg;
+	int dsm_call = (cmd == ND_CMD_CALL_DSM);
+
 	if (nvdimm) {
 		desc = nd_cmd_dimm_desc(cmd);
 		cmd_name = nvdimm_cmd_name(cmd);
-		dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0;
+		if (!dsm_call)
+			dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0;
 		dimm_name = dev_name(&nvdimm->dev);
 	} else {
 		desc = nd_cmd_bus_desc(cmd);
 		cmd_name = nvdimm_bus_cmd_name(cmd);
-		dsm_mask = nd_desc->dsm_mask;
+		if (!dsm_call)
+			dsm_mask = nd_desc->dsm_mask;
 		dimm_name = "bus";
 	}
 
@@ -517,6 +544,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		case ND_CMD_VENDOR:
 		case ND_CMD_SET_CONFIG_DATA:
 		case ND_CMD_ARS_START:
+		case ND_CMD_CALL_DSM:
 			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
@@ -544,6 +572,19 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		in_len += in_size;
 	}
 
+	if (dsm_call) {
+		pkg = (struct nd_cmd_dsmcall_pkg *) in_env;
+
+		dev_dbg(dev, "%s:%s rev: %llu, idx: %llu, in: %zu, out: %zu, len %zu\n",
+			__func__, dimm_name,
+			pkg->h.dsm_rev, pkg->h.dsm_fun_idx,
+			in_len, out_len, buf_len);
+
+		for (i = 0; i < ARRAY_SIZE(pkg->h.reserved2); i++)
+			if (pkg->h.reserved2[i])
+				return -EINVAL;
+	}
+
 	/* process an output envelope */
 	for (i = 0; i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
-- 
1.7.11.3

Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> 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

These look good to me.

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

Thanks Jerry!

Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> 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

These look good to me.

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

Thanks Jerry!

Re: [PATCH v5 1/6] ACPI / util: Fix acpi_evaluate_dsm() argument type

[Rafael J. Wysocki]

On Wednesday, January 06, 2016 04:03:38 PM Jerry Hoemann wrote:
> 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 <jerry.hoemann@hpe.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/utils.c    | 4 ++--
>  include/acpi/acpi_bus.h | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 475c907..049cba4 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -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;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ad0a5ff..8e6abcf 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -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;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v5 1/6] ACPI / util: Fix acpi_evaluate_dsm() argument type

[Rafael J. Wysocki]

On Wednesday, January 06, 2016 04:03:38 PM Jerry Hoemann wrote:
> 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 <jerry.hoemann@hpe.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/utils.c    | 4 ++--
>  include/acpi/acpi_bus.h | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 475c907..049cba4 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -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;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ad0a5ff..8e6abcf 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -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;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams <dan.j.williams@intel.com>wrote:
> On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
[..]
>> 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!

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.

Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams <dan.j.williams@intel.com>wrote:
> On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
[..]
>> 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!

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.

Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

On Sun, Jan 10, 2016 at 04:03:18PM -0800, Dan Williams wrote:
> On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams <dan.j.williams@intel.com>wrote:
> > On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> [..]
> >> 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!
> 
> 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.


  In sysfs?  okay that makes sense.


> 
> 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?


> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

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

Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

On Sun, Jan 10, 2016 at 04:03:18PM -0800, Dan Williams wrote:
> On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams <dan.j.williams@intel.com>wrote:
> > On Wed, Jan 6, 2016 at 3:03 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> [..]
> >> 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!
> 
> 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.


  In sysfs?  okay that makes sense.


> 
> 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?


> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

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

Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Mon, Jan 11, 2016 at 5:32 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Sun, Jan 10, 2016 at 04:03:18PM -0800, Dan Williams wrote:
>> On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams <dan.j.williams@intel.com>wrote:
[..]
>> 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.

True, but the set of uuids the kernel ever needs to know about is
likely small, and this policy mandates publication/notification of new
command sets to the kernel community.  Later on it gives the kernel a
touch point to implement dsm function number blacklisting which I
think is a useful security feature.

I'll leave the UUID parameter in the command in case a device ever
implements multiple command sets and we need to select between two
function number spaces.

>
>
>   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.

ACPICA will throw parse errors on mis-formatted DSMs.  We can't
prevent all malformed calls, but this is basic input validation that
the kernel can perform.

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

See nd_cmd_clear_to_send()... when a dimm is active the kernel
mandates that updates to the namespace labels go through sysfs.  This
is a safety measure to prevent userspace from inadvertently clobbering
in use labels.  Once the dimm goes idle (all 'region' devices related
to the dimm are disabled) userspace can manually update the
configuration data area.

Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Mon, Jan 11, 2016 at 5:32 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Sun, Jan 10, 2016 at 04:03:18PM -0800, Dan Williams wrote:
>> On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams <dan.j.williams@intel.com>wrote:
[..]
>> 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.

True, but the set of uuids the kernel ever needs to know about is
likely small, and this policy mandates publication/notification of new
command sets to the kernel community.  Later on it gives the kernel a
touch point to implement dsm function number blacklisting which I
think is a useful security feature.

I'll leave the UUID parameter in the command in case a device ever
implements multiple command sets and we need to select between two
function number spaces.

>
>
>   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.

ACPICA will throw parse errors on mis-formatted DSMs.  We can't
prevent all malformed calls, but this is basic input validation that
the kernel can perform.

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

See nd_cmd_clear_to_send()... when a dimm is active the kernel
mandates that updates to the namespace labels go through sysfs.  This
is a safety measure to prevent userspace from inadvertently clobbering
in use labels.  Once the dimm goes idle (all 'region' devices related
to the dimm are disabled) userspace can manually update the
configuration data area.