[no subject]
The original message was received at Mon, 13 Nov 2017 11:13:14 +0800 from lists.01.org [217.132.172.246] - The following addresses had permanent fatal errors -- Transcript of session follows - while talking to lists.01.org.: >>> MAIL From:"Bounced mail" <<< 501 "Bounced mail" ... Refused ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH 1/2] acpi, nfit: validate commands against the device type
Fix occasions in acpi_nfit_ctl where we check the command type without validating whether we are parsing dimm vs bus level commands. Where the command numbers alias between dimms and bus we can make the wrong assumption just checking the raw command number. For example, with a simple nfit_test mock up of the clear-error command we trigger the following: BUG: unable to handle kernel NULL pointer dereference at 0094 IP: acpi_nfit_ctl+0x29b/0x930 [nfit] [..] Call Trace: nfit_test_probe+0xb85/0xc09 [nfit_test] platform_drv_probe+0x3b/0xa0 ? platform_drv_probe+0x3b/0xa0 driver_probe_device+0x29c/0x450 ? test_alloc+0x180/0x180 [nfit_test] __driver_attach+0xe3/0xf0 ? driver_probe_device+0x450/0x450 bus_for_each_dev+0x73/0xc0 driver_attach+0x1e/0x20 bus_add_driver+0x173/0x270 driver_register+0x60/0xe0 __platform_driver_register+0x36/0x40 nfit_test_init+0x2a1/0x1000 [nfit_test] Fixes: 4b27db7e26cd ("acpi, nfit: add support for the _LSI, _LSR, and...") Reported-by: Vishal VermaSigned-off-by: Dan Williams --- drivers/acpi/nfit/core.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 8043bfde7c63..ff2580e7611d 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -483,13 +483,14 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, min_t(u32, 256, in_buf.buffer.length), true); /* call the BIOS, prefer the named methods over _DSM if available */ - if (cmd == ND_CMD_GET_CONFIG_SIZE && nfit_mem->has_lsi) + if (nvdimm && cmd == ND_CMD_GET_CONFIG_SIZE && nfit_mem->has_lsi) out_obj = acpi_label_info(handle); - else if (cmd == ND_CMD_GET_CONFIG_DATA && nfit_mem->has_lsr) { + else if (nvdimm && cmd == ND_CMD_GET_CONFIG_DATA && nfit_mem->has_lsr) { struct nd_cmd_get_config_data_hdr *p = buf; out_obj = acpi_label_read(handle, p->in_offset, p->in_length); - } else if (cmd == ND_CMD_SET_CONFIG_DATA && nfit_mem->has_lsw) { + } else if (nvdimm && cmd == ND_CMD_SET_CONFIG_DATA + && nfit_mem->has_lsw) { struct nd_cmd_set_config_hdr *p = buf; out_obj = acpi_label_write(handle, p->in_offset, p->in_length, @@ -497,7 +498,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, } else { u8 revid; - if (nfit_mem) + if (nvdimm) revid = nfit_dsm_revid(nfit_mem->family, func); else revid = 1; @@ -565,8 +566,10 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, * Set fw_status for all the commands with a known format to be * later interpreted by xlat_status(). */ - if (i >= 1 && ((cmd >= ND_CMD_ARS_CAP && cmd <= ND_CMD_CLEAR_ERROR) - || (cmd >= ND_CMD_SMART && cmd <= ND_CMD_VENDOR))) + if (i >= 1 && ((!nvdimm && cmd >= ND_CMD_ARS_CAP + && cmd <= ND_CMD_CLEAR_ERROR) + || (nvdimm && cmd >= ND_CMD_SMART + && cmd <= ND_CMD_VENDOR))) fw_status = *(u32 *) out_obj->buffer.pointer; if (offset + in_buf.buffer.length < buf_len) { ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH 2/2] tools/testing/nvdimm: unit test clear-error commands
Validate command parsing in acpi_nfit_ctl for the clear error command. This tests for a crash condition introduced by commit 4b27db7e26cd "acpi, nfit: add support for the _LSI, _LSR, and _LSW label methods". Cc: Vishal VermaSigned-off-by: Dan Williams --- tools/testing/nvdimm/test/nfit.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index edab68c2e221..7a4846d9607d 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -1812,6 +1812,7 @@ static int nfit_ctl_test(struct device *dev) unsigned long mask, cmd_size, offset; union { struct nd_cmd_get_config_size cfg_size; + struct nd_cmd_clear_error clear_err; struct nd_cmd_ars_status ars_stat; struct nd_cmd_ars_cap ars_cap; char buf[sizeof(struct nd_cmd_ars_status) @@ -1995,6 +1996,23 @@ static int nfit_ctl_test(struct device *dev) return -EIO; } + /* test clear error */ + cmd_size = sizeof(cmds.clear_err); + cmds.clear_err = (struct nd_cmd_clear_error) { + .length = 512, + .cleared = 512, + }; + rc = setup_result(cmds.buf, cmd_size); + if (rc) + return rc; + rc = acpi_nfit_ctl(_desc->nd_desc, NULL, ND_CMD_CLEAR_ERROR, + cmds.buf, cmd_size, _rc); + if (rc < 0 || cmd_rc >= 0) { + dev_dbg(dev, "%s: failed at: %d rc: %d cmd_rc: %d\n", + __func__, __LINE__, rc, cmd_rc); + return -EIO; + } + return 0; } ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] ndctl: daxctl: fix mmap size
On Sun, Nov 12, 2017 at 10:10 AM, Dan Williamswrote: > On Fri, Nov 10, 2017 at 11:45 AM, Dave Jiang wrote: >> The size for mmap needs to be aligned to the region alignment. Add helper >> funciton to determine the actual size to be mmap'd. >> >> Signed-off-by: Dave Jiang >> --- >> daxctl/io.c | 23 ++- >> 1 file changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/daxctl/io.c b/daxctl/io.c >> index 2f8cb4a..97a4169 100644 >> --- a/daxctl/io.c >> +++ b/daxctl/io.c >> @@ -80,9 +80,26 @@ static bool is_stdinout(struct io_dev *io_dev) >> io_dev->fd == STDOUT_FILENO) ? true : false; >> } >> >> +static int get_mmap_size(struct io_dev *io_dev, size_t size, size_t >> *map_size) >> +{ >> + unsigned long align; >> + >> + align = ndctl_dax_get_align(io_dev->dax); >> + if (align == ULLONG_MAX) >> + return -ERANGE; >> + >> + if (size <= align) >> + *map_size = align; >> + else >> + *map_size = (size / align) * align; > > We have the ALIGN() macro in util/size.h for this. > >> + >> + return 0; >> +} >> + >> static int setup_device(struct io_dev *io_dev, size_t size) >> { >> int flags, rc; >> + size_t map_size; >> >> if (is_stdinout(io_dev)) >> return 0; >> @@ -104,8 +121,12 @@ static int setup_device(struct io_dev *io_dev, size_t >> size) >> if (!io_dev->is_dax) >> return 0; >> >> + rc = get_mmap_size(io_dev, size, _size); >> + if (rc < 0) >> + return rc; >> + >> flags = (io_dev->direction == IO_READ) ? PROT_READ : PROT_WRITE; >> - io_dev->mmap = mmap(NULL, size, flags, MAP_SHARED, io_dev->fd, 0); >> + io_dev->mmap = mmap(NULL, map_size, flags, MAP_SHARED, io_dev->fd, >> 0); > > Why is offset 0 here? I would expect it to come from the command line > arguments and also use ALIGN_DOWN() to align to first aligned offset > before the target offset. I think we need some 'daxctl io' unit tests to catch cases like this. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] ndctl: daxctl: fix mmap size
On Fri, Nov 10, 2017 at 11:45 AM, Dave Jiangwrote: > The size for mmap needs to be aligned to the region alignment. Add helper > funciton to determine the actual size to be mmap'd. > > Signed-off-by: Dave Jiang > --- > daxctl/io.c | 23 ++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/daxctl/io.c b/daxctl/io.c > index 2f8cb4a..97a4169 100644 > --- a/daxctl/io.c > +++ b/daxctl/io.c > @@ -80,9 +80,26 @@ static bool is_stdinout(struct io_dev *io_dev) > io_dev->fd == STDOUT_FILENO) ? true : false; > } > > +static int get_mmap_size(struct io_dev *io_dev, size_t size, size_t > *map_size) > +{ > + unsigned long align; > + > + align = ndctl_dax_get_align(io_dev->dax); > + if (align == ULLONG_MAX) > + return -ERANGE; > + > + if (size <= align) > + *map_size = align; > + else > + *map_size = (size / align) * align; We have the ALIGN() macro in util/size.h for this. > + > + return 0; > +} > + > static int setup_device(struct io_dev *io_dev, size_t size) > { > int flags, rc; > + size_t map_size; > > if (is_stdinout(io_dev)) > return 0; > @@ -104,8 +121,12 @@ static int setup_device(struct io_dev *io_dev, size_t > size) > if (!io_dev->is_dax) > return 0; > > + rc = get_mmap_size(io_dev, size, _size); > + if (rc < 0) > + return rc; > + > flags = (io_dev->direction == IO_READ) ? PROT_READ : PROT_WRITE; > - io_dev->mmap = mmap(NULL, size, flags, MAP_SHARED, io_dev->fd, 0); > + io_dev->mmap = mmap(NULL, map_size, flags, MAP_SHARED, io_dev->fd, 0); Why is offset 0 here? I would expect it to come from the command line arguments and also use ALIGN_DOWN() to align to first aligned offset before the target offset. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm