From 9a901f5495e26e691c7d0ea7b6057a2f3e6330ed Mon Sep 17 00:00:00 2001 From: Vishal Verma Date: Mon, 5 Dec 2016 17:00:37 -0700 Subject: acpi, nfit: fix extended status translations for ACPI DSMs ACPI DSMs can have an 'extended' status which can be non-zero to convey additional information about the command. In the xlat_status routine, where we translate the command statuses, we were returning an error for a non-zero extended status, even if the primary status indicated success. Return from each command's 'case' once we have verified both its status and extend status are good. Cc: Fixes: 11294d63ac91 ("nfit: fail DSMs that return non-zero status by default") Signed-off-by: Vishal Verma Signed-off-by: Dan Williams --- drivers/acpi/nfit/core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 71a7d07c28c9..60acbb1d159c 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -113,7 +113,7 @@ static int xlat_status(void *buf, unsigned int cmd, u32 status) flags = ND_ARS_PERSISTENT | ND_ARS_VOLATILE; if ((status >> 16 & flags) == 0) return -ENOTTY; - break; + return 0; case ND_CMD_ARS_START: /* ARS is in progress */ if ((status & 0xffff) == NFIT_ARS_START_BUSY) @@ -122,7 +122,7 @@ static int xlat_status(void *buf, unsigned int cmd, u32 status) /* Command failed */ if (status & 0xffff) return -EIO; - break; + return 0; case ND_CMD_ARS_STATUS: ars_status = buf; /* Command failed */ @@ -154,7 +154,7 @@ static int xlat_status(void *buf, unsigned int cmd, u32 status) /* Unknown status */ if (status >> 16) return -EIO; - break; + return 0; case ND_CMD_CLEAR_ERROR: clear_err = buf; if (status & 0xffff) @@ -163,7 +163,7 @@ static int xlat_status(void *buf, unsigned int cmd, u32 status) return -EIO; if (clear_err->length > clear_err->cleared) return clear_err->cleared; - break; + return 0; default: break; } -- cgit v1.2.3 From efda1b5d87cbc3d8816f94a3815b413f1868e10d Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Tue, 6 Dec 2016 09:10:12 -0800 Subject: acpi, nfit, libnvdimm: fix / harden ars_status output length handling Given ambiguities in the ACPI 6.1 definition of the "Output (Size)" field of the ARS (Address Range Scrub) Status command, a firmware implementation may in practice return 0, 4, or 8 to indicate that there is no output payload to process. The specification states "Size of Output Buffer in bytes, including this field.". However, 'Output Buffer' is also the name of the entire payload, and earlier in the specification it states "Max Query ARS Status Output Buffer Size: Maximum size of buffer (including the Status and Extended Status fields)". Without this fix if the BIOS happens to return 0 it causes memory corruption as evidenced by this result from the acpi_nfit_ctl() unit test. ars_status00000000: 00020000 00000000 ........ BUG: stack guard page was hit at ffffc90001750000 (stack is ffffc9000174c000..ffffc9000174ffff) kernel stack overflow (page fault): 0000 [#1] SMP DEBUG_PAGEALLOC task: ffff8803332d2ec0 task.stack: ffffc9000174c000 RIP: 0010:[] [] __memcpy+0x12/0x20 RSP: 0018:ffffc9000174f9a8 EFLAGS: 00010246 RAX: ffffc9000174fab8 RBX: 0000000000000000 RCX: 000000001fffff56 RDX: 0000000000000000 RSI: ffff8803231f5a08 RDI: ffffc90001750000 RBP: ffffc9000174fa88 R08: ffffc9000174fab0 R09: ffff8803231f54b8 R10: 0000000000000008 R11: 0000000000000001 R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000003 R15: ffff8803231f54a0 FS: 00007f3a611af640(0000) GS:ffff88033ed00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffc90001750000 CR3: 0000000325b20000 CR4: 00000000000406e0 Stack: ffffffffa00bc60d 0000000000000008 ffffc90000000001 ffffc9000174faac 0000000000000292 ffffffffa00c24e4 ffffffffa00c2914 0000000000000000 0000000000000000 ffffffff00000003 ffff880331ae8ad0 0000000800000246 Call Trace: [] ? acpi_nfit_ctl+0x49d/0x750 [nfit] [] nfit_test_probe+0x670/0xb1b [nfit_test] Cc: Fixes: 747ffe11b440 ("libnvdimm, tools/testing/nvdimm: fix 'ars_status' output buffer sizing") Signed-off-by: Dan Williams --- drivers/acpi/nfit/core.c | 3 ++- drivers/nvdimm/bus.c | 25 ++++++++++++++++++++----- 2 files changed, 22 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 60acbb1d159c..e58ec32393b7 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -298,7 +298,8 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, for (i = 0, offset = 0; i < desc->out_num; i++) { u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i, buf, - (u32 *) out_obj->buffer.pointer); + (u32 *) out_obj->buffer.pointer, + out_obj->buffer.length - offset); if (offset + out_size > out_obj->buffer.length) { dev_dbg(dev, "%s:%s output object underflow cmd: %s field: %d\n", diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index a8b6949a8778..23d4a1728cdf 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -715,7 +715,7 @@ EXPORT_SYMBOL_GPL(nd_cmd_in_size); u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, const struct nd_cmd_desc *desc, int idx, const u32 *in_field, - const u32 *out_field) + const u32 *out_field, unsigned long remainder) { if (idx >= desc->out_num) return UINT_MAX; @@ -727,9 +727,24 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, return in_field[1]; else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) return out_field[1]; - else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 2) - return out_field[1] - 8; - else if (cmd == ND_CMD_CALL) { + else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 2) { + /* + * Per table 9-276 ARS Data in ACPI 6.1, out_field[1] is + * "Size of Output Buffer in bytes, including this + * field." + */ + if (out_field[1] < 4) + return 0; + /* + * ACPI 6.1 is ambiguous if 'status' is included in the + * output size. If we encounter an output size that + * overshoots the remainder by 4 bytes, assume it was + * including 'status'. + */ + if (out_field[1] - 8 == remainder) + return remainder; + return out_field[1] - 4; + } else if (cmd == ND_CMD_CALL) { struct nd_cmd_pkg *pkg = (struct nd_cmd_pkg *) in_field; return pkg->nd_size_out; @@ -876,7 +891,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, /* process an output envelope */ for (i = 0; i < desc->out_num; i++) { u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i, - (u32 *) in_env, (u32 *) out_env); + (u32 *) in_env, (u32 *) out_env, 0); u32 copy; if (out_size == UINT_MAX) { -- cgit v1.2.3 From 82aa37cf09867c5e2c0326649d570e5b25c1189a Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Tue, 6 Dec 2016 12:45:24 -0800 Subject: acpi, nfit: validate ars_status output buffer size If an ARS Status command returns truncated output, do not process partial records or otherwise consume non-status fields. Cc: Fixes: 0caeef63e6d2 ("libnvdimm: Add a poison list and export badblocks") Signed-off-by: Dan Williams --- drivers/acpi/nfit/core.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index e58ec32393b7..4b8b4f520d76 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -146,7 +146,8 @@ static int xlat_status(void *buf, unsigned int cmd, u32 status) * then just continue with the returned results. */ if (status == NFIT_ARS_STATUS_INTR) { - if (ars_status->flags & NFIT_ARS_F_OVERFLOW) + if (ars_status->out_length >= 40 && (ars_status->flags + & NFIT_ARS_F_OVERFLOW)) return -ENOSPC; return 0; } @@ -2002,19 +2003,32 @@ static int ars_get_status(struct acpi_nfit_desc *acpi_desc) return cmd_rc; } -static int ars_status_process_records(struct nvdimm_bus *nvdimm_bus, +static int ars_status_process_records(struct acpi_nfit_desc *acpi_desc, struct nd_cmd_ars_status *ars_status) { + struct nvdimm_bus *nvdimm_bus = acpi_desc->nvdimm_bus; int rc; u32 i; + /* + * First record starts at 44 byte offset from the start of the + * payload. + */ + if (ars_status->out_length < 44) + return 0; for (i = 0; i < ars_status->num_records; i++) { + /* only process full records */ + if (ars_status->out_length + < 44 + sizeof(struct nd_ars_record) * (i + 1)) + break; rc = nvdimm_bus_add_poison(nvdimm_bus, ars_status->records[i].err_address, ars_status->records[i].length); if (rc) return rc; } + if (i < ars_status->num_records) + dev_warn(acpi_desc->dev, "detected truncated ars results\n"); return 0; } @@ -2267,8 +2281,7 @@ static int acpi_nfit_query_poison(struct acpi_nfit_desc *acpi_desc, if (rc < 0 && rc != -ENOSPC) return rc; - if (ars_status_process_records(acpi_desc->nvdimm_bus, - acpi_desc->ars_status)) + if (ars_status_process_records(acpi_desc, acpi_desc->ars_status)) return -ENOMEM; return 0; -- cgit v1.2.3 From d6eb270c57fef35798525004ddf2ac5dcdadd43b Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Tue, 6 Dec 2016 15:06:55 -0800 Subject: acpi, nfit: fix bus vs dimm confusion in xlat_status Given dimms and bus commands share the same command number space we need to be careful that we are translating status in the correct context. Otherwise we can, for example, fail an ND_CMD_GET_CONFIG_SIZE command because max_xfer is zero. It fails because that condition erroneously correlates with the 'cleared == 0' failure of ND_CMD_CLEAR_ERROR. Cc: Fixes: aef253382266 ("libnvdimm, nfit: centralize command status translation") Signed-off-by: Dan Williams --- drivers/acpi/nfit/core.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 4b8b4f520d76..09d3db322ee8 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -94,7 +94,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc) return to_acpi_device(acpi_desc->dev); } -static int xlat_status(void *buf, unsigned int cmd, u32 status) +static int xlat_bus_status(void *buf, unsigned int cmd, u32 status) { struct nd_cmd_clear_error *clear_err; struct nd_cmd_ars_status *ars_status; @@ -175,6 +175,16 @@ static int xlat_status(void *buf, unsigned int cmd, u32 status) return 0; } +static int xlat_status(struct nvdimm *nvdimm, void *buf, unsigned int cmd, + u32 status) +{ + if (!nvdimm) + return xlat_bus_status(buf, cmd, status); + if (status) + return -EIO; + return 0; +} + static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc) @@ -335,7 +345,8 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, */ rc = buf_len - offset - in_buf.buffer.length; if (cmd_rc) - *cmd_rc = xlat_status(buf, cmd, fw_status); + *cmd_rc = xlat_status(nvdimm, buf, cmd, + fw_status); } else { dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n", __func__, dimm_name, cmd_name, buf_len, @@ -345,7 +356,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, } else { rc = 0; if (cmd_rc) - *cmd_rc = xlat_status(buf, cmd, fw_status); + *cmd_rc = xlat_status(nvdimm, buf, cmd, fw_status); } out: -- cgit v1.2.3 From a7de92dac9f0dbf01deb56fe1d661d7baac097e1 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Mon, 5 Dec 2016 13:43:25 -0800 Subject: tools/testing/nvdimm: unit test acpi_nfit_ctl() A recent flurry of bug discoveries in the nfit driver's DSM marshalling routine has highlighted the fact that we do not have unit test coverage for this routine. Add a self-test of acpi_nfit_ctl() routine before probing the "nfit_test.0" device. This mocks stimulus to acpi_nfit_ctl() and if any of the tests fail "nfit_test.0" will be unavailable causing the rest of the tests to not run / fail. This unit test will also be a place to land reproductions of quirky BIOS behavior discovered in the field and ensure the kernel does not regress against implementations it has seen in practice. Signed-off-by: Dan Williams --- drivers/acpi/nfit/core.c | 6 +++--- drivers/acpi/nfit/nfit.h | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 09d3db322ee8..312c4b4dc363 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -185,9 +185,8 @@ static int xlat_status(struct nvdimm *nvdimm, void *buf, unsigned int cmd, return 0; } -static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, - struct nvdimm *nvdimm, unsigned int cmd, void *buf, - unsigned int buf_len, int *cmd_rc) +int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, + unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc) { struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc); union acpi_object in_obj, in_buf, *out_obj; @@ -364,6 +363,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, return rc; } +EXPORT_SYMBOL_GPL(acpi_nfit_ctl); static const char *spa_type_name(u16 type) { diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h index 14296f5267c8..fc29c2e9832e 100644 --- a/drivers/acpi/nfit/nfit.h +++ b/drivers/acpi/nfit/nfit.h @@ -240,5 +240,7 @@ const u8 *to_nfit_uuid(enum nfit_uuids id); int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *nfit, acpi_size sz); void __acpi_nfit_notify(struct device *dev, acpi_handle handle, u32 event); void __acpi_nvdimm_notify(struct device *dev, u32 event); +int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, + unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc); void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev); #endif /* __NFIT_H__ */ -- cgit v1.2.3 From 325896ffdf90f7cbd59fb873b7ba20d60d1ddf3c Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Tue, 6 Dec 2016 17:03:35 -0800 Subject: device-dax: fix private mapping restriction, permit read-only Hugh notes in response to commit 4cb19355ea19 "device-dax: fail all private mapping attempts": "I think that is more restrictive than you intended: haven't tried, but I believe it rejects a PROT_READ, MAP_SHARED, O_RDONLY fd mmap, leaving no way to mmap /dev/dax without write permission to it." Indeed it does restrict read-only mappings, switch to checking VM_MAYSHARE, not VM_SHARED. Cc: Cc: Dave Hansen Cc: Pawel Lebioda Fixes: 4cb19355ea19 ("device-dax: fail all private mapping attempts") Reported-by: Hugh Dickins Signed-off-by: Dan Williams --- drivers/dax/dax.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c index 3d94ff20fdca..286447a83dab 100644 --- a/drivers/dax/dax.c +++ b/drivers/dax/dax.c @@ -271,7 +271,7 @@ static int check_vma(struct dax_dev *dax_dev, struct vm_area_struct *vma, return -ENXIO; /* prevent private mappings from being established */ - if ((vma->vm_flags & VM_SHARED) != VM_SHARED) { + if ((vma->vm_flags & VM_MAYSHARE) != VM_MAYSHARE) { dev_info(dev, "%s: %s: fail, attempted private mapping\n", current->comm, func); return -EINVAL; -- cgit v1.2.3