On Fri, Apr 14, 2023 at 12:12:37PM +0200, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 12:01:15 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “nd_pfn_validate”.
>
> Thus avoid the risk for undefined behaviour by replacing the usage of
> the local variable “parent_uuid” by a direct function call within
> a later condition check.
Hi Markus,
I think I understand what you are saying above, but I don't follow
how that applies here. This change seems to be a nice simplification,
parent_uuid, is used once, just grab it when needed.
What is the risk of undefined behavior?
>
> This issue was detected by using the Coccinelle software.
Which cocci script?
>
> Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")
This fixes tag seems to be the wrong tag. It is a tag from when the
uuid helpers were introduce, not where parent_uuid was first introduced
and used. I'm not clear this warrants a Fixes tag anyway. Is there
really a bug here? Perhaps I'm missing something in the previous
explanation of risk.
checkpatch is WARNING on the tag format:
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: d1c6e08e7503 ("libnvdimm/labels: Add uuid helpers")'
#17:
Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")
checkpatch is also WARNING on the commit msg:
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#5:
nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
Also, possible only my pet peeve, the long commit message spoils my
pretty 80 column view. Please trim it to not wrap here:
$git log --oneline pfn_devs.c
52b639e56a46 nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
c91d71363084 nvdimm: Support sizeof(struct page) > MAX_STRUCT_PAGE_SIZE
6e9f05dc66f9 libnvdimm/pfn_dev: increase MAX_STRUCT_PAGE_SIZE
81beea55cb74 nvdimm: Drop nd_device_lock()
4a0079bc7aae nvdimm: Replace lockdep_mutex with local lock classes
322cbb50de71 block: remove genhd.h
Alison
> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/nvdimm/pfn_devs.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index af7d9301520c..f14cbfa500ed 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -456,7 +456,6 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
> unsigned long align, start_pad;
> struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
> struct nd_namespace_common *ndns = nd_pfn->ndns;
> - const uuid_t *parent_uuid = nd_dev_to_uuid(&ndns->dev);
>
> if (!pfn_sb || !ndns)
> return -ENODEV;
> @@ -476,7 +475,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
> return -ENODEV;
> pfn_sb->checksum = cpu_to_le64(checksum);
>
> - if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
> + if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(&ndns->dev), 16) != 0)
> return -ENODEV;
>
> if (__le16_to_cpu(pfn_sb->version_minor) < 1) {
> --
> 2.40.0
>
