[bug report] ACPI: NFIT: Define runtime firmware activation commands
Hello Dan Williams,
The patch 6450ddbd5d8e: "ACPI: NFIT: Define runtime firmware
activation commands" from Jul 20, 2020, leads to the following static
checker warning:
drivers/acpi/nfit/core.c:481 acpi_nfit_ctl()
error: passing untrusted data 'family' to 'test_bit()'
drivers/acpi/nfit/core.c:483 acpi_nfit_ctl()
warn: uncapped user index 'acpi_desc->family_dsm_mask[family]'
drivers/acpi/nfit/core.c
435 int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm
*nvdimm,
436 unsigned int cmd, void *buf, unsigned int buf_len, int
*cmd_rc)
437 {
438 struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
439 struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
440 union acpi_object in_obj, in_buf, *out_obj;
441 const struct nd_cmd_desc *desc = NULL;
442 struct device *dev = acpi_desc->dev;
443 struct nd_cmd_pkg *call_pkg = NULL;
444 const char *cmd_name, *dimm_name;
445 unsigned long cmd_mask, dsm_mask;
446 u32 offset, fw_status = 0;
447 acpi_handle handle;
448 const guid_t *guid;
449 int func, rc, i;
450 int family = 0;
451
452 if (cmd_rc)
453 *cmd_rc = -EINVAL;
454
455 if (cmd == ND_CMD_CALL)
456 call_pkg = buf;
^^^
If cmd == ND_CMD_CALL then call_pkg is controlled by the user.
457 func = cmd_to_func(nfit_mem, cmd, call_pkg, );
cmd_to_func() checks "call_pkg->nd_family" but only if nfit_mem is
non-NULL.
458 if (func < 0)
459 return func;
460
461 if (nvdimm) {
462 struct acpi_device *adev = nfit_mem->adev;
463
464 if (!adev)
465 return -ENOTTY;
466
467 dimm_name = nvdimm_name(nvdimm);
468 cmd_name = nvdimm_cmd_name(cmd);
469 cmd_mask = nvdimm_cmd_mask(nvdimm);
470 dsm_mask = nfit_mem->dsm_mask;
471 desc = nd_cmd_dimm_desc(cmd);
472 guid = to_nfit_uuid(nfit_mem->family);
473 handle = adev->handle;
474 } else {
475 struct acpi_device *adev = to_acpi_dev(acpi_desc);
476
477 cmd_name = nvdimm_bus_cmd_name(cmd);
478 cmd_mask = nd_desc->cmd_mask;
479 if (cmd == ND_CMD_CALL && call_pkg->nd_family) {
480 family = call_pkg->nd_family;
481 if (!test_bit(family,
_desc->bus_family_mask))
^^
if "family" is more BITS_PER_LONG then this will overflow.
482 return -EINVAL;
483 dsm_mask = acpi_desc->family_dsm_mask[family];
^^^
484 guid = to_nfit_bus_uuid(family);
485 } else {
486 dsm_mask = acpi_desc->bus_dsm_mask;
487 guid = to_nfit_uuid(NFIT_DEV_BUS);
488 }
489 desc = nd_cmd_bus_desc(cmd);
490 handle = adev->handle;
491 dimm_name = "bus";
492 }
493
494 if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
495 return -ENOTTY;
496
497 /*
498 * Check for a valid command. For ND_CMD_CALL, we also have to
499 * make sure that the DSM function is supported.
500 */
501 if (cmd == ND_CMD_CALL &&
regards,
dan carpenter
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[bug report] device-dax: add dis-contiguous resource support
Hello Dan Williams,
This is a semi-automatic email about new static checker warnings.
The patch 454c727769f5: "device-dax: add dis-contiguous resource
support" from Aug 26, 2020, leads to the following Smatch complaint:
drivers/dax/bus.c:788 alloc_dev_dax_range()
error: we previously assumed 'alloc' could be null (see line 772)
drivers/dax/bus.c
771 alloc = __request_region(res, start, size, dev_name(dev), 0);
772 if (!alloc && !dev_dax->nr_range) {
^^
This should probably be a ||?
773 /*
774 * If we adjusted an existing @ranges leave it alone,
775 * but if this was an empty set of ranges nothing else
776 * will release @ranges, so do it now.
777 */
778 kfree(ranges);
779 return -ENOMEM;
780 }
781
782 for (i = 0; i < dev_dax->nr_range; i++)
783 pgoff += PHYS_PFN(range_len([i].range));
784 dev_dax->ranges = ranges;
785 ranges[dev_dax->nr_range++] = (struct dev_dax_range) {
786 .pgoff = pgoff,
787 .range = {
788 .start = alloc->start,
Dereferences.
789 .end = alloc->end,
790 },
regards,
dan carpenter
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[PATCH 1/2] acpi/nfit: improve bounds checking for 'func'
The 'func' variable can come from the user in the __nd_ioctl(). If it's
too high then the (1 << func) shift in acpi_nfit_clear_to_send() is
undefined. In acpi_nfit_ctl() we pass 'func' to test_bit(func, _mask)
which could result in an out of bounds access.
To fix these issues, I introduced the NVDIMM_CMD_MAX (31) define and
updated nfit_dsm_revid() to use that define as well instead of magic
numbers.
Fixes: 11189c1089da ("acpi/nfit: Fix command-supported detection")
Signed-off-by: Dan Carpenter
---
drivers/acpi/nfit/core.c | 10 ++
drivers/acpi/nfit/nfit.h | 1 +
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 24241941181c..b317f4043705 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -34,6 +34,7 @@
| 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 \
(1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index a3320f93616d..d0090f71585c 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -360,7 +360,7 @@ static union acpi_object *acpi_label_info(acpi_handle
handle)
static u8 nfit_dsm_revid(unsigned family, unsigned func)
{
- static const u8 revid_table[NVDIMM_FAMILY_MAX+1][32] = {
+ static const u8 revid_table[NVDIMM_FAMILY_MAX+1][NVDIMM_CMD_MAX+1] = {
[NVDIMM_FAMILY_INTEL] = {
[NVDIMM_INTEL_GET_MODES] = 2,
[NVDIMM_INTEL_GET_FWINFO] = 2,
@@ -386,7 +386,7 @@ static u8 nfit_dsm_revid(unsigned family, unsigned func)
if (family > NVDIMM_FAMILY_MAX)
return 0;
- if (func > 31)
+ if (func > NVDIMM_CMD_MAX)
return 0;
id = revid_table[family][func];
if (id == 0)
@@ -492,7 +492,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm,
* Check for a valid command. For ND_CMD_CALL, we also have to
* make sure that the DSM function is supported.
*/
- if (cmd == ND_CMD_CALL && !test_bit(func, _mask))
+ if (cmd == ND_CMD_CALL &&
+ (func > NVDIMM_CMD_MAX || !test_bit(func, _mask)))
return -ENOTTY;
else if (!test_bit(cmd, _mask))
return -ENOTTY;
@@ -3492,7 +3493,8 @@ static int acpi_nfit_clear_to_send(struct
nvdimm_bus_descriptor *nd_desc,
if (nvdimm && cmd == ND_CMD_CALL &&
call_pkg->nd_family == NVDIMM_FAMILY_INTEL) {
func = call_pkg->nd_command;
- if ((1 << func) & NVDIMM_INTEL_SECURITY_CMDMASK)
+ if (func > NVDIMM_CMD_MAX ||
+ (1 << func) & NVDIMM_INTEL_SECURITY_CMDMASK)
return -EOPNOTSUPP;
}
--
2.11.0
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[PATCH 2/2] libnvdimm: Out of bounds read in __nd_ioctl()
The "cmd" comes from the user and it can be up to 255. It it's more
than the number of bits in long, it results out of bounds read when we
check test_bit(cmd, _mask). The highest valid value for "cmd" is
ND_CMD_CALL (10) so I added a compare against that.
Fixes: 62232e45f4a2 ("libnvdimm: control (ioctl) messages for nvdimm_bus and
nvdimm devices")
Signed-off-by: Dan Carpenter
---
drivers/nvdimm/bus.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index a8b515968569..09087c38fabd 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -1042,8 +1042,10 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus,
struct nvdimm *nvdimm,
return -EFAULT;
}
- if (!desc || (desc->out_num + desc->in_num == 0) ||
- !test_bit(cmd, _mask))
+ if (!desc ||
+ (desc->out_num + desc->in_num == 0) ||
+ cmd > ND_CMD_CALL ||
+ !test_bit(cmd, _mask))
return -ENOTTY;
/* fail write commands (when read-only) */
--
2.11.0
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [bug report] libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver infrastructure
On Wed, Feb 05, 2020 at 12:04:15PM -0800, Dan Williams wrote: > On Wed, Feb 5, 2020 at 11:28 AM Dan Carpenter > wrote: > > > > On Wed, Feb 05, 2020 at 11:16:22AM -0800, Dan Williams wrote: > > > Ugh, sorry I thought you were pointing out that there's too many > > > put_device() not the use after free. Yes, the use after free is a bug > > > that needs fixing. > > > > I am complaining about the device_puts... If we call device_put() > > twice then it cause a problem in __nvdimm_create() > > > > drivers/nvdimm/dimm_devs.c > >506 nvdimm->sec.flags = nvdimm_security_flags(nvdimm, > > NVDIMM_USER); > >507 nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, > > NVDIMM_MASTER); > >508 nd_device_register(dev); > >509 > >510 return nvdimm; > >^^ > > If we call device_put() twice then we this pointer within 4 seconds. > > "we this pointer"? We "what" this pointer. 4 seconds is relative to a > runtime test case? > Sorry. I meant we *free* it. The second device_put() leads to a nvdimm_release(dev) where dev is ">dev" within 0-4 seconds. Most times it will free it immediately but if you have CONFIG_DEBUG_KOBJECT_RELEASE enabled then it will wait between 1-4 seconds and then free nvdimm. It's a config option, not a runtime thing. regards, dan carpenter ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [bug report] libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver infrastructure
On Wed, Feb 05, 2020 at 11:16:22AM -0800, Dan Williams wrote:
> Ugh, sorry I thought you were pointing out that there's too many
> put_device() not the use after free. Yes, the use after free is a bug
> that needs fixing.
I am complaining about the device_puts... If we call device_put()
twice then it cause a problem in __nvdimm_create()
drivers/nvdimm/dimm_devs.c
506 nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
507 nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm,
NVDIMM_MASTER);
508 nd_device_register(dev);
509
510 return nvdimm;
^^
If we call device_put() twice then we this pointer within 4 seconds.
511 }
The fix is probably to make nd_device_register() return an error code so
we can do:
ret = nd_device_register(dev);
if (ret) {
device_put(>dev);
return NULL;
}
return nvdimm;
regards,
dan carpenter
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [bug report] libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver infrastructure
On Wed, Feb 05, 2020 at 10:23:00AM -0800, Dan Williams wrote:
> > > >506 if (device_add(dev) != 0) {
> > > >507 dev_err(dev, "%s: failed\n", __func__);
> > > >508 put_device(dev);
> > > > ^^^
> > > >509 }
> > > >510 put_device(dev);
> > > > ^^
> > > >511 if (dev->parent)
> > > >512 put_device(dev->parent);
> > > >513 }
> > > >
> > > > We call get_device() from __nd_device_register(), I guess. It seems
> > > > buggy to call put device twice on error.
> > >
> > > The registration path does:
> > >
> > > get_device(dev);
> > >
> > > async_schedule_dev_domain(nd_async_device_register, dev,
> > > _async_domain);
> > >
> > > ...and device_add() does its own get_device().
> >
> > device_add() does its own put_device() at the end so it's a net zero.
> >
>
> It does it's own, yes, but the put_device() after device_add() failure
> is there to drop the reference taken by device_initialize().
> Otherwise, device_add() has always documented:
>
> * NOTE: _Never_ directly free @dev after calling this function, even
> * if it returned an error! Always use put_device() to give up your
> * reference instead.
>
> ...so what am I missing?
The "never call kfree" is hopefully straight forward because the kobject
needs to do its own cleanup.
__nvdimm_create() allocates the dev.
nd_device_register() calls device_initialize() which call kobject_init()
so the refcount is 1.
__nd_device_register() call get_device() so the refcount is now two.
nd_async_device_register() decrements the refcount once on success.
But if device_add() fails then it decrements it twice. Now the refcount
is zero so we call nvdimm_release(). This leads to a use after free on
the next line:
put_device(dev);
if (dev->parent)
There is a trick here because depending on the debug options it
might free immediately or it might call nvdimm_release() after 4
seconds. See kobject_release() for details.
Either way if device_add() fails we return back to __nvdimm_create()
and return the zero reference count "nvdimm" pointer, which is going
to be a problem.
regards,
dan carpenter
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [bug report] libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver infrastructure
On Wed, Feb 05, 2020 at 09:47:01AM -0800, Dan Williams wrote:
> On Wed, Feb 5, 2020 at 4:38 AM Dan Carpenter wrote:
> >
> > Hello Dan Williams,
> >
> > The patch 4d88a97aa9e8: "libnvdimm, nvdimm: dimm driver and base
> > libnvdimm device-driver infrastructure" from May 31, 2015, leads to
> > the following static checker warning:
> >
> > drivers/nvdimm/bus.c:511 nd_async_device_register()
> > error: dereferencing freed memory 'dev'
> >
> > drivers/nvdimm/bus.c
> >502 static void nd_async_device_register(void *d, async_cookie_t cookie)
> >503 {
> >504 struct device *dev = d;
> >505
> >506 if (device_add(dev) != 0) {
> >507 dev_err(dev, "%s: failed\n", __func__);
> >508 put_device(dev);
> > ^^^
> >509 }
> >510 put_device(dev);
> > ^^
> >511 if (dev->parent)
> >512 put_device(dev->parent);
> >513 }
> >
> > We call get_device() from __nd_device_register(), I guess. It seems
> > buggy to call put device twice on error.
>
> The registration path does:
>
> get_device(dev);
>
> async_schedule_dev_domain(nd_async_device_register, dev,
> _async_domain);
>
> ...and device_add() does its own get_device().
device_add() does its own put_device() at the end so it's a net zero.
regards,
dan carpenter
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[bug report] libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver infrastructure
Hello Dan Williams,
The patch 4d88a97aa9e8: "libnvdimm, nvdimm: dimm driver and base
libnvdimm device-driver infrastructure" from May 31, 2015, leads to
the following static checker warning:
drivers/nvdimm/bus.c:511 nd_async_device_register()
error: dereferencing freed memory 'dev'
drivers/nvdimm/bus.c
502 static void nd_async_device_register(void *d, async_cookie_t cookie)
503 {
504 struct device *dev = d;
505
506 if (device_add(dev) != 0) {
507 dev_err(dev, "%s: failed\n", __func__);
508 put_device(dev);
^^^
509 }
510 put_device(dev);
^^
511 if (dev->parent)
512 put_device(dev->parent);
513 }
We call get_device() from __nd_device_register(), I guess. It seems
buggy to call put device twice on error.
regards,
dan carpenter
regards,
dan carpenter
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: KASAN: vmalloc-out-of-bounds Read in acpi_nfit_ctl
ND_CMD_CALL && !test_bit(func, _mask))
If func is more than sizeof(long) * 8 then this will overflow. The
temptation is to add a check on func in cmd_to_func() but capping it at
sizeof(long) * 8 feels unnatural and I'm not sure what the max function
should be.
[Edit. I see below that > 31 is not supported. ]
496 return -ENOTTY;
497 else if (!test_bit(cmd, _mask))
498 return -ENOTTY;
499
500 in_obj.type = ACPI_TYPE_PACKAGE;
501 in_obj.package.count = 1;
502 in_obj.package.elements = _buf;
There is a another problem in acpi_nfit_clear_to_send().
acpi/nfit/core.c
3485 /* prevent security commands from being issued via ioctl */
3486 static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor
*nd_desc,
3487 struct nvdimm *nvdimm, unsigned int cmd, void *buf)
3488 {
3489 struct nd_cmd_pkg *call_pkg = buf;
3490 unsigned int func;
3491
3492 if (nvdimm && cmd == ND_CMD_CALL &&
3493 call_pkg->nd_family == NVDIMM_FAMILY_INTEL) {
3494 func = call_pkg->nd_command;
3495 if ((1 << func) & NVDIMM_INTEL_SECURITY_CMDMASK)
^^^^^
This is undefined if func is greater than 31.
3496 return -EOPNOTSUPP;
3497 }
3498
3499 return __acpi_nfit_clear_to_send(nd_desc, nvdimm, cmd);
3500 }
regards,
dan carpenter
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[PATCH] acpi/nfit: unlock on error in scrub_show()
We change the locking in this function and forgot to update this error
path so we are accidentally still holding the "dev->lockdep_mutex".
Fixes: 87a30e1f05d7 ("driver-core, libnvdimm: Let device subsystems add local
lockdep coverage")
Signed-off-by: Dan Carpenter
---
drivers/acpi/nfit/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 1413324982f0..14e68f202f81 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1322,7 +1322,7 @@ static ssize_t scrub_show(struct device *dev,
nfit_device_lock(dev);
nd_desc = dev_get_drvdata(dev);
if (!nd_desc) {
- device_unlock(dev);
+ nfit_device_unlock(dev);
return rc;
}
acpi_desc = to_acpi_desc(nd_desc);
--
2.20.1
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH V3] libnvdimm/namsepace: Don't set claim_class on error
On Wed, Sep 25, 2019 at 02:13:48PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > Don't leave claim_class set to an invalid value if an error occurs in > btt_claim_class(). > > While we are here change the return type of __holder_class_store() to be > clear about the values it is returning. > > This was found via code inspection. > > Reported-by: Dan Carpenter > Reviewed-by: Vishal Verma > Signed-off-by: Ira Weiny > > --- > V1->V2 > Add space after variable declaration... > > V2->V3 > Fix oneliner > Rebase without Dan Carpenter's patch and give him Reported-by > credit Thanks! Btw, if it's ever a question of "do you want to redo a patch or just transfer to reported by credit?" then always I always want the #2 option. It would have taken me several iterations to generate the patch you wanted. regards, dan carpenter ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] libnvdimm/namespace: Fix a signedness bug in __holder_class_store()
The "ndns->claim_class" variable is an enum but in this case GCC will
treat it as an unsigned int so the error handling is never triggered.
Fixes: 14e494542636 ("libnvdimm, btt: BTT updates for UEFI 2.7 format")
Signed-off-by: Dan Carpenter
---
drivers/nvdimm/namespace_devs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index cca0a3ba1d2c..669985527716 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1529,7 +1529,7 @@ static ssize_t __holder_class_store(struct device *dev,
const char *buf)
return -EINVAL;
/* btt_claim_class() could've returned an error */
- if (ndns->claim_class < 0)
+ if ((int)ndns->claim_class < 0)
return ndns->claim_class;
return 0;
--
2.20.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile
On Fri, Sep 13, 2019 at 05:44:39PM -0400, Martin K. Petersen wrote: > One pet peeve I have is that people are pretty bad at indicating the > intended target tree. I often ask for it in private mail but the > practice doesn't seem to stick. I spend a ton of time guessing whether a > patch is a fix for a new feature in the x+1 queue or a fix for the > current release. It is not always obvious. The Fixes tag doesn't help? Of course, you've never asked me or anyone on kernel-newbies that question. We don't normally know the answer either. I do always try to figure it out for networking, but it's kind of a pain in the butt and I mess up surpisingly often for how much effort I put into getting it right. regards, dan carpenter ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile
On Fri, Sep 13, 2019 at 01:09:37AM -0600, Jonathan Corbet wrote: > On Wed, 11 Sep 2019 16:11:29 -0600 > Jens Axboe wrote: > > > On 9/11/19 12:43 PM, Dan Carpenter wrote: > > > > > > I kind of hate all this extra documentation because now everyone thinks > > > they can invent new hoops to jump through. > > > > FWIW, I completely agree with Dan (Carpenter) here. I absolutely > > dislike having these kinds of files, and with subsystems imposing weird > > restrictions on style (like the quoted example, yuck). > > > > Additionally, it would seem saner to standardize rules around when > > code is expected to hit the maintainers hands for kernel releases. Both > > yours and Martins deals with that, there really shouldn't be the need > > to have this specified in detail per sub-system. > > This sort of objection came up at the maintainers summit yesterday; the > consensus was that, while we might not like subsystem-specific rules, they > do currently exist and we're just documenting reality. To paraphrase > Phillip K. Dick, reality is that which, when you refuse to document it, > doesn't go away. There aren't that many subsystem rules. The big exception is networking, with the comment style and reverse Chrismas tree declarations. Also you have to label which git tree the patch applies to like [net] or [net-next]. It used to be that infiniband used "sizeof foo" instead of sizeof(foo) but now there is a new maintainer. There is one subsystem which where the maintainer will capitalize your patch prefix and complain. There are others where they will silently change it to lower case. (Maybe that has changed in recent years). There is one subsystem where the maintainer is super strict rules that you can't use "I" or "we" in the commit message. So you can't say "I noticed a bug while reviewing", you have to say "The code has a bug". Some maintainers have rules about what you can put in the declaration block. No kmalloc() in the declarations is a common rule. "struct foo *p = kmalloc();". Some people (I do) have strict rules for error handling, but most won't complain unless the error handling has bugs. The bpf people want you to put [bpf] or [bpf-next] in the subject. Everyone just guesses, and uneducated guesses are worse than leaving it blank, but that's just my opinion. > So I'm expecting to take this kind of stuff into Documentation/. My own > personal hope is that it can maybe serve to shame some of these "local > quirks" out of existence. The evidence from this brief discussion suggests > that this might indeed happen. I don't think it's shaming, I think it's validating. Everyone just insists that since it's written in the Book of Rules then it's our fault for not reading it. It's like those EULA things where there is more text than anyone can physically read in a life time. And the documentation doesn't help. For example, I knew people's rules about capitalizing the subject but I'd just forget. I say that if you can't be bothered to add it to checkpatch then it means you don't really care that strongly. regards, dan carpenter ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile
On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
> +Coding Style Addendum
> +-
> +libnvdimm expects multi-line statements to be double indented. I.e.
> +
> +if (x...
> +&& ...y) {
That looks horrible and it causes a checkpatch warning. :( Why not
do it the same way that everyone else does it.
if (blah_blah_x && <-- && has to be on the first line for checkpatch
blah_blah_y) { <-- [tab][space][space][space][space]blah
Now all the conditions are aligned visually which makes it readable.
They aren't aligned with the indent block so it's easy to tell the
inside from the if condition.
I kind of hate all this extra documentation because now everyone thinks
they can invent new hoops to jump through.
Speaking of hoops, the BPF documentation says that you have to figure
out which tree a patch applies to instead of just working against
linux-next. Is there an automated way to do that? I looked through my
inbox and there are a bunch of patches marked as going through the
bpf-next tree but about half were marked incorrectly so it looks like
everyone who tries to tag their patches is going to do it badly anyway.
regards,
dan carpenter
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[bug report] acpi/nfit: Add support for Intel DSM 1.8 commands
i, buf);
519
520 if (call_pkg) {
521 /* skip over package wrapper */
522 in_buf.buffer.pointer = (void *) _pkg->nd_payload;
523 in_buf.buffer.length = call_pkg->nd_size_in;
524 }
525
526 dev_dbg(dev, "%s cmd: %d: func: %d input length: %d\n",
527 dimm_name, cmd, func, in_buf.buffer.length);
528 if (payload_dumpable(nvdimm, func))
[ snip ]
3500 /* prevent security commands from being issued via ioctl */
3501 static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor
*nd_desc,
3502 struct nvdimm *nvdimm, unsigned int cmd, void *buf)
3503 {
3504 struct nd_cmd_pkg *call_pkg = buf;
3505 unsigned int func;
3506
3507 if (nvdimm && cmd == ND_CMD_CALL &&
3508 call_pkg->nd_family == NVDIMM_FAMILY_INTEL) {
3509 func = call_pkg->nd_command;
3510 if ((1 << func) & NVDIMM_INTEL_SECURITY_CMDMASK)
^
This is undefined and it would be nice to fix, but not important for
run time.
3511 return -EOPNOTSUPP;
3512 }
3513
3514 return __acpi_nfit_clear_to_send(nd_desc, nvdimm, cmd);
3515 }
regards,
dan carpenter
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v4 08/17] kunit: test: add support for test abort
On Thu, Feb 28, 2019 at 01:03:24AM -0800, Brendan Higgins wrote:
> you could do:
>
> if (IS_ERR_OR_NULL(ptr)) {
> KUNIT_FAIL(test, "ptr is an errno or null: %ld", ptr);
> return;
> }
It's best to not mix error pointers and NULL but when we do mix them,
it means that NULL is a special kind of success. Like we try to load
a feature and we get back:
valid pointer <-- success
null <-- feature is disabled. not an error.
error pointer <-- feature is broken. fail.
regards,
dan carpenter
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[bug report] acpi/nfit: fix cmd_rc for acpi_nfit_ctl to always return a value
2 }
503
504 if (!out_obj) {
505 dev_dbg(dev, "%s _DSM failed cmd: %s\n", dimm_name,
cmd_name);
506 return -EINVAL;
507 }
508
509 if (call_pkg) {
510 call_pkg->nd_fw_size = out_obj->buffer.length;
511 memcpy(call_pkg->nd_payload + call_pkg->nd_size_in,
512 out_obj->buffer.pointer,
513 min(call_pkg->nd_fw_size,
call_pkg->nd_size_out));
514
515 ACPI_FREE(out_obj);
516 /*
517 * Need to support FW function w/o known size in
advance.
518 * Caller can determine required size based upon
nd_fw_size.
519 * If we return an error (like elsewhere) then caller
wouldn't
520 * be able to rely upon data returned to make
calculation.
521 */
522 *cmd_rc = 0;
523 return 0;
524 }
525
526 if (out_obj->package.type != ACPI_TYPE_BUFFER) {
527 dev_dbg(dev, "%s unexpected output object type cmd: %s
type: %d\n",
528 dimm_name, cmd_name, out_obj->type);
529 rc = -EINVAL;
530 goto out;
531 }
532
533 dev_dbg(dev, "%s cmd: %s output length: %d\n", dimm_name,
534 cmd_name, out_obj->buffer.length);
535 print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4, 4,
536 out_obj->buffer.pointer,
537 min_t(u32, 128, out_obj->buffer.length), true);
538
539 for (i = 0, offset = 0; i < desc->out_num; i++) {
540 u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
buf,
541 (u32 *) out_obj->buffer.pointer,
542 out_obj->buffer.length - offset);
543
544 if (offset + out_size > out_obj->buffer.length) {
545 dev_dbg(dev, "%s output object underflow cmd:
%s field: %d\n",
546 dimm_name, cmd_name, i);
547 break;
548 }
549
550 if (in_buf.buffer.length + offset + out_size > buf_len)
{
551 dev_dbg(dev, "%s output overrun cmd: %s field:
%d\n",
552 dimm_name, cmd_name, i);
553 rc = -ENXIO;
554 goto out;
555 }
556 memcpy(buf + in_buf.buffer.length + offset,
557 out_obj->buffer.pointer + offset,
out_size);
558 offset += out_size;
559 }
560
561 /*
562 * Set fw_status for all the commands with a known format to be
563 * later interpreted by xlat_status().
564 */
565 if (i >= 1 && ((!nvdimm && cmd >= ND_CMD_ARS_CAP
566 && cmd <= ND_CMD_CLEAR_ERROR)
567 || (nvdimm && cmd >= ND_CMD_SMART
568 && cmd <= ND_CMD_VENDOR)))
569 fw_status = *(u32 *) out_obj->buffer.pointer;
570
571 if (offset + in_buf.buffer.length < buf_len) {
572 if (i >= 1) {
573 /*
574 * status valid, return the number of bytes left
575 * unfilled in the output buffer
576 */
577 rc = buf_len - offset - in_buf.buffer.length;
578 if (cmd_rc)
^^
Checked too late.
579 *cmd_rc = xlat_status(nvdimm, buf, cmd,
580 fw_status);
regards,
dan carpenter
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[bug report] libnvdimm: add support for clear poison list and badblocks for device dax
Hello Dave Jiang,
The patch 006358b35c73: "libnvdimm: add support for clear poison list
and badblocks for device dax" from Apr 7, 2017, leads to the
following static checker warning:
drivers/nvdimm/bus.c:852 nd_pmem_forget_poison_check()
warn: we tested 'nd_dax' before and it was 'false'
drivers/nvdimm/bus.c
835 static int nd_pmem_forget_poison_check(struct device *dev, void *data)
836 {
837 struct nd_cmd_clear_error *clear_err =
838 (struct nd_cmd_clear_error *)data;
839 struct nd_btt *nd_btt = is_nd_btt(dev) ? to_nd_btt(dev) : NULL;
840 struct nd_pfn *nd_pfn = is_nd_pfn(dev) ? to_nd_pfn(dev) : NULL;
841 struct nd_dax *nd_dax = is_nd_dax(dev) ? to_nd_dax(dev) : NULL;
^^
nd_dax is set here.
842 struct nd_namespace_common *ndns = NULL;
843 struct nd_namespace_io *nsio;
844 resource_size_t offset = 0, end_trunc = 0, start, end, pstart,
pend;
845
846 if (nd_dax || !dev->driver)
^^
We return if it's non-NULL
847 return 0;
848
849 start = clear_err->address;
850 end = clear_err->address + clear_err->cleared - 1;
851
852 if (nd_btt || nd_pfn || nd_dax) {
853 if (nd_btt)
854 ndns = nd_btt->ndns;
855 else if (nd_pfn)
856 ndns = nd_pfn->ndns;
857 else if (nd_dax)
^^
but the rest of the function assumes it can be true. Perhaps we plan
to enable it in the future? It's not clear to me.
858 ndns = nd_dax->nd_pfn.ndns;
859
860 if (!ndns)
861 return 0;
862 } else
863 ndns = to_ndns(dev);
864
865 nsio = to_nd_namespace_io(>dev);
866 pstart = nsio->res.start + offset;
867 pend = nsio->res.end - end_trunc;
868
869 if ((pstart >= start) && (pend <= end))
870 return -EBUSY;
871
872 return 0;
873
874 }
regards,
dan carpenter
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[bug report] libnvdimm: clear the internal poison_list when clearing badblocks
Hello Vishal Verma,
The patch e046114af5fc: "libnvdimm: clear the internal poison_list
when clearing badblocks" from Sep 30, 2016, leads to the following
static checker warning:
drivers/nvdimm/core.c:601 nvdimm_forget_poison()
warn: potential integer overflow from user 'start + len'
drivers/nvdimm/core.c
597 void nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus, phys_addr_t
start,
598 unsigned int len)
599 {
600 struct list_head *poison_list = _bus->poison_list;
601 u64 clr_end = start + len - 1;
^^^
Thes come from the __nd_ioctl() and it looks like they haven't been
checked before we call this function. It's hard for me to read this
function well enough that I can say for sure the overflow is harmless.
Please review?
602 struct nd_poison *pl, *next;
603
604 spin_lock(_bus->poison_lock);
605 WARN_ON_ONCE(list_empty(poison_list));
606
607 /*
608 * [start, clr_end] is the poison interval being cleared.
609 * [pl->start, pl_end] is the poison_list entry we're comparing
610 * the above interval against. The poison list entry may need
611 * to be modified (update either start or length), deleted, or
612 * split into two based on the overlap characteristics
613 */
614
615 list_for_each_entry_safe(pl, next, poison_list, list) {
616 u64 pl_end = pl->start + pl->length - 1;
617
618 /* Skip intervals with no intersection */
619 if (pl_end < start)
620 continue;
621 if (pl->start > clr_end)
622 continue;
623 /* Delete completely overlapped poison entries */
624 if ((pl->start >= start) && (pl_end <= clr_end)) {
625 list_del(>list);
regards,
dan carpenter
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[bug report] libnvdimm: control (ioctl) messages for nvdimm_bus and nvdimm devices
Hello Dan Williams,
The patch 62232e45f4a2: "libnvdimm: control (ioctl) messages for
nvdimm_bus and nvdimm devices" from Jun 8, 2015, leads to the
following static checker warning:
drivers/nvdimm/bus.c:1018 __nd_ioctl()
warn: integer overflows 'buf_len'
drivers/nvdimm/bus.c
959 /* process an input envelope */
960 for (i = 0; i < desc->in_num; i++) {
961 u32 in_size, copy;
962
963 in_size = nd_cmd_in_size(nvdimm, cmd, desc, i, in_env);
964 if (in_size == UINT_MAX) {
965 dev_err(dev, "%s:%s unknown input size cmd: %s
field: %d\n",
966 __func__, dimm_name, cmd_name,
i);
967 return -ENXIO;
968 }
969 if (in_len < sizeof(in_env))
970 copy = min_t(u32, sizeof(in_env) - in_len,
in_size);
971 else
972 copy = 0;
973 if (copy && copy_from_user(_env[in_len], p + in_len,
copy))
974 return -EFAULT;
975 in_len += in_size;
>From a casual review, this seems like it might be a real bug. On the
first iteration we load some data into in_env[]. On the second
iteration we read a use controlled "in_size" from nd_cmd_in_size(). It
can go up to UINT_MAX - 1. A high number means we will fill the whole
in_env[] buffer. But we potentially keep looping and adding more to
in_len so now it can be any value.
It simple enough to change:
- in_len += in_size;
+ in_len += copy;
But it feels weird that we keep looping even though in_env is totally
full. Shouldn't we just return an error if we don't have space for
desc->in_num.
976 }
977
978 if (cmd == ND_CMD_CALL) {
979 func = pkg.nd_command;
980 dev_dbg(dev, "%s:%s, idx: %llu, in: %zu, out: %zu, len
%zu\n",
981 __func__, dimm_name, pkg.nd_command,
982 in_len, out_len, buf_len);
983
984 for (i = 0; i < ARRAY_SIZE(pkg.nd_reserved2); i++)
985 if (pkg.nd_reserved2[i])
986 return -EINVAL;
987 }
988
989 /* process an output envelope */
990 for (i = 0; i < desc->out_num; i++) {
991 u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
992 (u32 *) in_env, (u32 *) out_env, 0);
993 u32 copy;
994
995 if (out_size == UINT_MAX) {
996 dev_dbg(dev, "%s:%s unknown output size cmd: %s
field: %d\n",
997 __func__, dimm_name, cmd_name,
i);
998 return -EFAULT;
999 }
1000 if (out_len < sizeof(out_env))
1001 copy = min_t(u32, sizeof(out_env) - out_len,
out_size);
1002 else
1003 copy = 0;
1004 if (copy && copy_from_user(_env[out_len],
1005 p + in_len + out_len, copy))
1006 return -EFAULT;
1007 out_len += out_size;
Same thing.
1008 }
1009
1010 buf_len = out_len + in_len;
It means this addition could overflow.
1011 if (buf_len > ND_IOCTL_MAX_BUFLEN) {
1012 dev_dbg(dev, "%s:%s cmd: %s buf_len: %zu > %d\n",
__func__,
1013 dimm_name, cmd_name, buf_len,
1014 ND_IOCTL_MAX_BUFLEN);
1015 return -EINVAL;
1016 }
1017
1018 buf = vmalloc(buf_len);
^^^
The checker complains we might allocate less than intended.
1019 if (!buf)
1020 return -ENOMEM;
1021
1022 if (copy_from_user(buf, p, buf_len)) {
1023 rc = -EFAULT;
1024 goto out;
1025 }
regards,
dan carpenter
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[patch] libnvdimm, namespace: potential NULL deref on allocation error
If the kcalloc() fails then "devs" can be NULL and we dereference it
checking "devs[i]".
Fixes: 1b40e09a1232 ('libnvdimm: blk labels and namespace instantiation')
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 3509cff..abe5c6b 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -2176,12 +2176,14 @@ static struct device **scan_labels(struct nd_region
*nd_region)
return devs;
err:
- for (i = 0; devs[i]; i++)
- if (is_nd_blk(_region->dev))
- namespace_blk_release(devs[i]);
- else
- namespace_pmem_release(devs[i]);
- kfree(devs);
+ if (devs) {
+ for (i = 0; devs[i]; i++)
+ if (is_nd_blk(_region->dev))
+ namespace_blk_release(devs[i]);
+ else
+ namespace_pmem_release(devs[i]);
+ kfree(devs);
+ }
return NULL;
}
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
