libnvdimm: Validate command family indices
diff mbox series

Message ID 158593318491.130477.12103487421973195234.stgit@dwillia2-desk3.amr.corp.intel.com
State New, archived
Headers show
Series
  • libnvdimm: Validate command family indices
Related show

Commit Message

Dan Williams April 3, 2020, 4:59 p.m. UTC
The ND_CMD_CALL format allows for a general passthrough of whitelisted
commands targeting a given command set. However there is no validation
of the family index relative to what the bus supports.

- Update the NFIT bus implementation (the only one that supports
  ND_CMD_CALL passthrough) to also whitelist the valid set of command
  family indices.

- Update the generic __nd_ioctl() path to validate that field on behalf
  of all implementations.

Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c   |   11 +++++++++--
 drivers/acpi/nfit/nfit.h   |    1 -
 drivers/nvdimm/bus.c       |   16 ++++++++++++++++
 include/linux/libnvdimm.h  |    2 ++
 include/uapi/linux/ndctl.h |    4 ++++
 5 files changed, 31 insertions(+), 3 deletions(-)

Comments

Vaibhav Jain April 3, 2020, 7:35 p.m. UTC | #1
Hi Dan,

This sounds like good idea and would reduce some cmd_pkg checking overhead in
nvdimm and bus modules. However have few concerns regarding the proposed
PAPR_SCM command family below

Dan Williams <dan.j.williams@intel.com> writes:

> The ND_CMD_CALL format allows for a general passthrough of whitelisted
> commands targeting a given command set. However there is no validation
> of the family index relative to what the bus supports.
>
> - Update the NFIT bus implementation (the only one that supports
>   ND_CMD_CALL passthrough) to also whitelist the valid set of command
>   family indices.
PAPR_SCM command family will also use ND_CMD_CALL passthrough and will
need to be whitelist even though it doesnt use NFIT.

>
> - Update the generic __nd_ioctl() path to validate that field on behalf
>   of all implementations.
>
> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/acpi/nfit/core.c   |   11 +++++++++--
>  drivers/acpi/nfit/nfit.h   |    1 -
>  drivers/nvdimm/bus.c       |   16 ++++++++++++++++
>  include/linux/libnvdimm.h  |    2 ++
>  include/uapi/linux/ndctl.h |    4 ++++
>  5 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index d0090f71585c..bcf5af803941 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1823,6 +1823,7 @@ static void populate_shutdown_status(struct nfit_mem *nfit_mem)
>  static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  		struct nfit_mem *nfit_mem, u32 device_handle)
>  {
> +	struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
>  	struct acpi_device *adev, *adev_dimm;
>  	struct device *dev = acpi_desc->dev;
>  	unsigned long dsm_mask, label_mask;
> @@ -1834,6 +1835,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  	/* nfit test assumes 1:1 relationship between commands and dsms */
>  	nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
>  	nfit_mem->family = NVDIMM_FAMILY_INTEL;
> +	set_bit(NVDIMM_FAMILY_INTEL, &nd_desc->dimm_family_mask);
>  
>  	if (dcr->valid_fields & ACPI_NFIT_CONTROL_MFG_INFO_VALID)
>  		sprintf(nfit_mem->id, "%04x-%02x-%04x-%08x",
> @@ -1886,10 +1888,13 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>  	 * Note, that checking for function0 (bit0) tells us if any commands
>  	 * are reachable through this GUID.
>  	 */
> +	clear_bit(NVDIMM_FAMILY_INTEL, &nd_desc->dimm_family_mask);
>  	for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)
> -		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
> +		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
> +			set_bit(i, &nd_desc->dimm_family_mask);
>  			if (family < 0 || i == default_dsm_family)
>  				family = i;
> +		}
>  
>  	/* limit the supported commands to those that are publicly documented */
>  	nfit_mem->family = family;
> @@ -2151,6 +2156,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>  
>  	nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>  	nd_desc->bus_dsm_mask = acpi_desc->bus_nfit_cmd_force_en;
> +	set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
> +	set_bit(NVDIMM_BUS_FAMILY_NFIT, &nd_desc->bus_family_mask);
> +
>  	adev = to_acpi_dev(acpi_desc);
>  	if (!adev)
>  		return;
> @@ -2158,7 +2166,6 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>  	for (i = ND_CMD_ARS_CAP; i <= ND_CMD_CLEAR_ERROR; i++)
>  		if (acpi_check_dsm(adev->handle, guid, 1, 1ULL << i))
>  			set_bit(i, &nd_desc->cmd_mask);
> -	set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
>  
>  	dsm_mask =
>  		(1 << ND_CMD_ARS_CAP) |
> diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
> index b317f4043705..5f5f8ce030e7 100644
> --- a/drivers/acpi/nfit/nfit.h
> +++ b/drivers/acpi/nfit/nfit.h
> @@ -33,7 +33,6 @@
>  		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
>  		| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
>  
> -#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV
>  #define NVDIMM_CMD_MAX 31
>  
>  #define NVDIMM_STANDARD_CMDMASK \
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 09087c38fabd..955265656b96 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -1037,9 +1037,25 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>  		dimm_name = "bus";
>  	}
>  
> +	/* Validate command family support against bus declared support */
>  	if (cmd == ND_CMD_CALL) {
> +		unsigned long *mask;
> +
>  		if (copy_from_user(&pkg, p, sizeof(pkg)))
>  			return -EFAULT;
> +
> +		if (nvdimm) {
> +			if (pkg.nd_family > NVDIMM_FAMILY_MAX)
> +				return -EINVAL;
> +			mask = &nd_desc->dimm_family_mask;
> +		} else {
> +			if (pkg.nd_family > NVDIMM_BUS_FAMILY_MAX)
> +				return -EINVAL;
> +			mask = &nd_desc->bus_family_mask;
> +		}
> +
> +		if (!test_bit(pkg.nd_family, mask))
> +			return -EINVAL;
>  	}
>  
>  	if (!desc ||
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 9df091bd30ba..b41857f43883 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -76,6 +76,8 @@ struct nvdimm_bus_descriptor {
>  	const struct attribute_group **attr_groups;
>  	unsigned long bus_dsm_mask;
>  	unsigned long cmd_mask;
> +	unsigned long dimm_family_mask;
> +	unsigned long bus_family_mask;
>  	struct module *module;
>  	char *provider_name;
>  	struct device_node *of_node;
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index de5d90212409..e28763c234e2 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -244,6 +244,10 @@ struct nd_cmd_pkg {
>  #define NVDIMM_FAMILY_HPE2 2
>  #define NVDIMM_FAMILY_MSFT 3
>  #define NVDIMM_FAMILY_HYPERV 4
> +#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV

How do you envision support for a new command family that doesnt
support NFIT like PAPR_SCM, be added to this list ?

As I see the value NVDIMM_FAMILY_MAX will be tied to the set of command
families supported by nfit and if PAPR_SCM command family is added at
index 5 then should or should-not the NVDIMM_FAMILY_MAX be updated to
value 5.

If NVDIMM_FAMILY_MAX is not updated then __nd_ioctl() wont let PAPR-SCM
commands pass through. However if NVDIMM_FAMILY_MAX is updated then nfit
module will wrongly advertise support for PAPR-SCM command family.

> +
> +#define NVDIMM_BUS_FAMILY_NFIT 0
> +#define NVDIMM_BUS_FAMILY_MAX NVDIMM_BUS_FAMILY_NFIT
>  
>  #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
>  					struct nd_cmd_pkg)
>
Thanks,

Patch
diff mbox series

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index d0090f71585c..bcf5af803941 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1823,6 +1823,7 @@  static void populate_shutdown_status(struct nfit_mem *nfit_mem)
 static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 		struct nfit_mem *nfit_mem, u32 device_handle)
 {
+	struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
 	struct acpi_device *adev, *adev_dimm;
 	struct device *dev = acpi_desc->dev;
 	unsigned long dsm_mask, label_mask;
@@ -1834,6 +1835,7 @@  static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	/* nfit test assumes 1:1 relationship between commands and dsms */
 	nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
 	nfit_mem->family = NVDIMM_FAMILY_INTEL;
+	set_bit(NVDIMM_FAMILY_INTEL, &nd_desc->dimm_family_mask);
 
 	if (dcr->valid_fields & ACPI_NFIT_CONTROL_MFG_INFO_VALID)
 		sprintf(nfit_mem->id, "%04x-%02x-%04x-%08x",
@@ -1886,10 +1888,13 @@  static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	 * Note, that checking for function0 (bit0) tells us if any commands
 	 * are reachable through this GUID.
 	 */
+	clear_bit(NVDIMM_FAMILY_INTEL, &nd_desc->dimm_family_mask);
 	for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)
-		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
+		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
+			set_bit(i, &nd_desc->dimm_family_mask);
 			if (family < 0 || i == default_dsm_family)
 				family = i;
+		}
 
 	/* limit the supported commands to those that are publicly documented */
 	nfit_mem->family = family;
@@ -2151,6 +2156,9 @@  static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 
 	nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
 	nd_desc->bus_dsm_mask = acpi_desc->bus_nfit_cmd_force_en;
+	set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+	set_bit(NVDIMM_BUS_FAMILY_NFIT, &nd_desc->bus_family_mask);
+
 	adev = to_acpi_dev(acpi_desc);
 	if (!adev)
 		return;
@@ -2158,7 +2166,6 @@  static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 	for (i = ND_CMD_ARS_CAP; i <= ND_CMD_CLEAR_ERROR; i++)
 		if (acpi_check_dsm(adev->handle, guid, 1, 1ULL << i))
 			set_bit(i, &nd_desc->cmd_mask);
-	set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
 
 	dsm_mask =
 		(1 << ND_CMD_ARS_CAP) |
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index b317f4043705..5f5f8ce030e7 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -33,7 +33,6 @@ 
 		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
 		| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
 
-#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV
 #define NVDIMM_CMD_MAX 31
 
 #define NVDIMM_STANDARD_CMDMASK \
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 09087c38fabd..955265656b96 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -1037,9 +1037,25 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		dimm_name = "bus";
 	}
 
+	/* Validate command family support against bus declared support */
 	if (cmd == ND_CMD_CALL) {
+		unsigned long *mask;
+
 		if (copy_from_user(&pkg, p, sizeof(pkg)))
 			return -EFAULT;
+
+		if (nvdimm) {
+			if (pkg.nd_family > NVDIMM_FAMILY_MAX)
+				return -EINVAL;
+			mask = &nd_desc->dimm_family_mask;
+		} else {
+			if (pkg.nd_family > NVDIMM_BUS_FAMILY_MAX)
+				return -EINVAL;
+			mask = &nd_desc->bus_family_mask;
+		}
+
+		if (!test_bit(pkg.nd_family, mask))
+			return -EINVAL;
 	}
 
 	if (!desc ||
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 9df091bd30ba..b41857f43883 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -76,6 +76,8 @@  struct nvdimm_bus_descriptor {
 	const struct attribute_group **attr_groups;
 	unsigned long bus_dsm_mask;
 	unsigned long cmd_mask;
+	unsigned long dimm_family_mask;
+	unsigned long bus_family_mask;
 	struct module *module;
 	char *provider_name;
 	struct device_node *of_node;
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index de5d90212409..e28763c234e2 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -244,6 +244,10 @@  struct nd_cmd_pkg {
 #define NVDIMM_FAMILY_HPE2 2
 #define NVDIMM_FAMILY_MSFT 3
 #define NVDIMM_FAMILY_HYPERV 4
+#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV
+
+#define NVDIMM_BUS_FAMILY_NFIT 0
+#define NVDIMM_BUS_FAMILY_MAX NVDIMM_BUS_FAMILY_NFIT
 
 #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
 					struct nd_cmd_pkg)