* [ndctl PATCH 0/2] fix two issues reported by Coverity
@ 2021-06-15 12:37 Zhiqiang Liu
2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Zhiqiang Liu @ 2021-06-15 12:37 UTC (permalink / raw)
To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26
Recently, we use Coverity to analysis the ndctl package, again.
Two issues should be resolved to make Coverity happy.
Zhiqiang Liu (2):
libndctl: check return value of ndctl_pfn_get_namespace
namespace: fix potentail fd leak problem in do_xaction_namespace()
ndctl/namespace.c | 35 +++++++++++++++++++++++------------
test/libndctl.c | 4 ++--
util/json.c | 2 ++
3 files changed, 27 insertions(+), 14 deletions(-)
--
2.23.0
.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace
2021-06-15 12:37 [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu
@ 2021-06-15 12:38 ` Zhiqiang Liu
2021-06-30 17:41 ` Alison Schofield
2021-06-15 12:39 ` [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() Zhiqiang Liu
2021-06-30 2:55 ` [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu
2 siblings, 1 reply; 9+ messages in thread
From: Zhiqiang Liu @ 2021-06-15 12:38 UTC (permalink / raw)
To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26
ndctl_pfn_get_namespace() may return NULL, so callers
should check return value of it. Otherwise, it may
cause access NULL pointer problem.
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
ndctl/namespace.c | 18 ++++++++++++++----
test/libndctl.c | 4 ++--
util/json.c | 2 ++
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 0c8df9f..21089d7 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -1417,11 +1417,16 @@ static int nstype_clear_badblocks(struct ndctl_namespace *ndns,
static int dax_clear_badblocks(struct ndctl_dax *dax)
{
- struct ndctl_namespace *ndns = ndctl_dax_get_namespace(dax);
- const char *devname = ndctl_dax_get_devname(dax);
+ struct ndctl_namespace *ndns;
+ const char *devname;
unsigned long long begin, size;
int rc;
+ ndns = ndctl_dax_get_namespace(dax);
+ if (!ndns)
+ return -ENXIO;
+
+ devname = ndctl_dax_get_devname(dax);
begin = ndctl_dax_get_resource(dax);
if (begin == ULLONG_MAX)
return -ENXIO;
@@ -1441,11 +1446,16 @@ static int dax_clear_badblocks(struct ndctl_dax *dax)
static int pfn_clear_badblocks(struct ndctl_pfn *pfn)
{
- struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
- const char *devname = ndctl_pfn_get_devname(pfn);
+ struct ndctl_namespace *ndns;
+ const char *devname;
unsigned long long begin, size;
int rc;
+ ndns = ndctl_pfn_get_namespace(pfn);
+ if (!ndns)
+ return -ENXIO;
+
+ devname = ndctl_pfn_get_devname(pfn);
begin = ndctl_pfn_get_resource(pfn);
if (begin == ULLONG_MAX)
return -ENXIO;
diff --git a/test/libndctl.c b/test/libndctl.c
index 24d72b3..05e5ff2 100644
--- a/test/libndctl.c
+++ b/test/libndctl.c
@@ -1275,7 +1275,7 @@ static int check_pfn_autodetect(struct ndctl_bus *bus,
if (!ndctl_pfn_is_enabled(pfn))
continue;
pfn_ndns = ndctl_pfn_get_namespace(pfn);
- if (strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
+ if (!pfn_ndns || strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
continue;
fprintf(stderr, "%s: pfn_ndns: %p ndns: %p\n", __func__,
pfn_ndns, ndns);
@@ -1372,7 +1372,7 @@ static int check_dax_autodetect(struct ndctl_bus *bus,
if (!ndctl_dax_is_enabled(dax))
continue;
dax_ndns = ndctl_dax_get_namespace(dax);
- if (strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
+ if (!dax_ndns || strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
continue;
fprintf(stderr, "%s: dax_ndns: %p ndns: %p\n", __func__,
dax_ndns, ndns);
diff --git a/util/json.c b/util/json.c
index ca0167b..249f021 100644
--- a/util/json.c
+++ b/util/json.c
@@ -1002,6 +1002,8 @@ static struct json_object *util_pfn_badblocks_to_json(struct ndctl_pfn *pfn,
pfn_begin = ndctl_pfn_get_resource(pfn);
if (pfn_begin == ULLONG_MAX) {
struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
+ if (!ndns)
+ return NULL;
return util_namespace_badblocks_to_json(ndns, bb_count, flags);
}
--
2.23.0
.
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace()
2021-06-15 12:37 [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu
2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
@ 2021-06-15 12:39 ` Zhiqiang Liu
2021-06-30 17:47 ` Alison Schofield
2021-06-30 2:55 ` [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu
2 siblings, 1 reply; 9+ messages in thread
From: Zhiqiang Liu @ 2021-06-15 12:39 UTC (permalink / raw)
To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26
In do_xaction_namespace(), ri_ctx.f_out should be closed after
being opened.
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
ndctl/namespace.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 21089d7..55364ac 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -2141,7 +2141,7 @@ static int do_xaction_namespace(const char *namespace,
util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
if (rc >= 0)
(*processed)++;
- return rc;
+ goto out;
}
}
@@ -2152,11 +2152,11 @@ static int do_xaction_namespace(const char *namespace,
rc = file_write_infoblock(param.outfile);
if (rc >= 0)
(*processed)++;
- return rc;
+ goto out;
}
if (!namespace && action != ACTION_CREATE)
- return rc;
+ goto out;
if (verbose)
ndctl_set_log_priority(ctx, LOG_DEBUG);
@@ -2212,7 +2212,7 @@ static int do_xaction_namespace(const char *namespace,
saved_rc = rc;
continue;
}
- return rc;
+ goto out;
}
ndctl_namespace_foreach_safe(region, ndns, _n) {
ndns_name = ndctl_namespace_get_devname(ndns);
@@ -2259,7 +2259,7 @@ static int do_xaction_namespace(const char *namespace,
rc = namespace_reconfig(region, ndns);
if (rc == 0)
*processed = 1;
- return rc;
+ goto out;
case ACTION_READ_INFOBLOCK:
rc = namespace_rw_infoblock(ndns, &ri_ctx, READ);
if (rc == 0)
@@ -2281,9 +2281,6 @@ static int do_xaction_namespace(const char *namespace,
if (ri_ctx.jblocks)
util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
- if (ri_ctx.f_out && ri_ctx.f_out != stdout)
- fclose(ri_ctx.f_out);
-
if (action == ACTION_CREATE && rc == -EAGAIN) {
/*
* Namespace creation searched through all candidate
@@ -2301,6 +2298,10 @@ static int do_xaction_namespace(const char *namespace,
if (saved_rc)
rc = saved_rc;
+out:
+ if (ri_ctx.f_out && ri_ctx.f_out != stdout)
+ fclose(ri_ctx.f_out);
+
return rc;
}
--
2.23.0
.
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH 0/2] fix two issues reported by Coverity
2021-06-15 12:37 [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu
2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
2021-06-15 12:39 ` [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() Zhiqiang Liu
@ 2021-06-30 2:55 ` Zhiqiang Liu
2 siblings, 0 replies; 9+ messages in thread
From: Zhiqiang Liu @ 2021-06-30 2:55 UTC (permalink / raw)
To: vishal.l.verma; +Cc: nvdimm, linfeilong, lixiaokeng, liuzhiqiang26
friendly ping..
On 2021/6/15 20:37, Zhiqiang Liu wrote:
> Recently, we use Coverity to analysis the ndctl package, again.
> Two issues should be resolved to make Coverity happy.
>
> Zhiqiang Liu (2):
> libndctl: check return value of ndctl_pfn_get_namespace
> namespace: fix potentail fd leak problem in do_xaction_namespace()
>
> ndctl/namespace.c | 35 +++++++++++++++++++++++------------
> test/libndctl.c | 4 ++--
> util/json.c | 2 ++
> 3 files changed, 27 insertions(+), 14 deletions(-)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace
2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
@ 2021-06-30 17:41 ` Alison Schofield
2021-07-01 3:23 ` Zhiqiang Liu
0 siblings, 1 reply; 9+ messages in thread
From: Alison Schofield @ 2021-06-30 17:41 UTC (permalink / raw)
To: Zhiqiang Liu; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng
On Tue, Jun 15, 2021 at 08:38:33PM +0800, Zhiqiang Liu wrote:
>
> ndctl_pfn_get_namespace() may return NULL, so callers
> should check return value of it. Otherwise, it may
> cause access NULL pointer problem.
>
Hi Zhiqiang,
I see you mentioned this was found by Coverity in the cover letter.
Please repeat that in the commit log here.
What about the call path:
ndctl_dax_get_namespace() --> ndctl_pfn_get_namespace()
Seems like another place where ndctl_pfn_get_namespace() could
eventually lead to a NULL ptr dereference.
Alison
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> ndctl/namespace.c | 18 ++++++++++++++----
> test/libndctl.c | 4 ++--
> util/json.c | 2 ++
> 3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index 0c8df9f..21089d7 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -1417,11 +1417,16 @@ static int nstype_clear_badblocks(struct ndctl_namespace *ndns,
>
> static int dax_clear_badblocks(struct ndctl_dax *dax)
> {
> - struct ndctl_namespace *ndns = ndctl_dax_get_namespace(dax);
> - const char *devname = ndctl_dax_get_devname(dax);
> + struct ndctl_namespace *ndns;
> + const char *devname;
> unsigned long long begin, size;
> int rc;
>
> + ndns = ndctl_dax_get_namespace(dax);
> + if (!ndns)
> + return -ENXIO;
> +
> + devname = ndctl_dax_get_devname(dax);
> begin = ndctl_dax_get_resource(dax);
> if (begin == ULLONG_MAX)
> return -ENXIO;
> @@ -1441,11 +1446,16 @@ static int dax_clear_badblocks(struct ndctl_dax *dax)
>
> static int pfn_clear_badblocks(struct ndctl_pfn *pfn)
> {
> - struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
> - const char *devname = ndctl_pfn_get_devname(pfn);
> + struct ndctl_namespace *ndns;
> + const char *devname;
> unsigned long long begin, size;
> int rc;
>
> + ndns = ndctl_pfn_get_namespace(pfn);
> + if (!ndns)
> + return -ENXIO;
> +
> + devname = ndctl_pfn_get_devname(pfn);
> begin = ndctl_pfn_get_resource(pfn);
> if (begin == ULLONG_MAX)
> return -ENXIO;
> diff --git a/test/libndctl.c b/test/libndctl.c
> index 24d72b3..05e5ff2 100644
> --- a/test/libndctl.c
> +++ b/test/libndctl.c
> @@ -1275,7 +1275,7 @@ static int check_pfn_autodetect(struct ndctl_bus *bus,
> if (!ndctl_pfn_is_enabled(pfn))
> continue;
> pfn_ndns = ndctl_pfn_get_namespace(pfn);
> - if (strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
> + if (!pfn_ndns || strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
> continue;
> fprintf(stderr, "%s: pfn_ndns: %p ndns: %p\n", __func__,
> pfn_ndns, ndns);
> @@ -1372,7 +1372,7 @@ static int check_dax_autodetect(struct ndctl_bus *bus,
> if (!ndctl_dax_is_enabled(dax))
> continue;
> dax_ndns = ndctl_dax_get_namespace(dax);
> - if (strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
> + if (!dax_ndns || strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
> continue;
> fprintf(stderr, "%s: dax_ndns: %p ndns: %p\n", __func__,
> dax_ndns, ndns);
> diff --git a/util/json.c b/util/json.c
> index ca0167b..249f021 100644
> --- a/util/json.c
> +++ b/util/json.c
> @@ -1002,6 +1002,8 @@ static struct json_object *util_pfn_badblocks_to_json(struct ndctl_pfn *pfn,
> pfn_begin = ndctl_pfn_get_resource(pfn);
> if (pfn_begin == ULLONG_MAX) {
> struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
> + if (!ndns)
> + return NULL;
>
> return util_namespace_badblocks_to_json(ndns, bb_count, flags);
> }
> --
> 2.23.0
>
>
>
>
> .
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace()
2021-06-15 12:39 ` [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() Zhiqiang Liu
@ 2021-06-30 17:47 ` Alison Schofield
2021-07-01 1:42 ` Zhiqiang Liu
0 siblings, 1 reply; 9+ messages in thread
From: Alison Schofield @ 2021-06-30 17:47 UTC (permalink / raw)
To: Zhiqiang Liu; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng
On Tue, Jun 15, 2021 at 08:39:20PM +0800, Zhiqiang Liu wrote:
>
> In do_xaction_namespace(), ri_ctx.f_out should be closed after
> being opened.
>
Hi Zhiqiang,
The commit message and commit log need to be swapped.
Something like:
Commit message says what the patch does:
[ndctl PATCH 2/2] namespace: Close fd before return in do_xaction_namespace()
Commit log says why it needs to be done:
This prevents a potential file descriptor leak in do_xaction_namespace()
And, same as in Patch 1 - mention it was found by Coverity.
Alison
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> ndctl/namespace.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index 21089d7..55364ac 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -2141,7 +2141,7 @@ static int do_xaction_namespace(const char *namespace,
> util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
> if (rc >= 0)
> (*processed)++;
> - return rc;
> + goto out;
> }
> }
>
> @@ -2152,11 +2152,11 @@ static int do_xaction_namespace(const char *namespace,
> rc = file_write_infoblock(param.outfile);
> if (rc >= 0)
> (*processed)++;
> - return rc;
> + goto out;
> }
>
> if (!namespace && action != ACTION_CREATE)
> - return rc;
> + goto out;
>
> if (verbose)
> ndctl_set_log_priority(ctx, LOG_DEBUG);
> @@ -2212,7 +2212,7 @@ static int do_xaction_namespace(const char *namespace,
> saved_rc = rc;
> continue;
> }
> - return rc;
> + goto out;
> }
> ndctl_namespace_foreach_safe(region, ndns, _n) {
> ndns_name = ndctl_namespace_get_devname(ndns);
> @@ -2259,7 +2259,7 @@ static int do_xaction_namespace(const char *namespace,
> rc = namespace_reconfig(region, ndns);
> if (rc == 0)
> *processed = 1;
> - return rc;
> + goto out;
> case ACTION_READ_INFOBLOCK:
> rc = namespace_rw_infoblock(ndns, &ri_ctx, READ);
> if (rc == 0)
> @@ -2281,9 +2281,6 @@ static int do_xaction_namespace(const char *namespace,
> if (ri_ctx.jblocks)
> util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
>
> - if (ri_ctx.f_out && ri_ctx.f_out != stdout)
> - fclose(ri_ctx.f_out);
> -
> if (action == ACTION_CREATE && rc == -EAGAIN) {
> /*
> * Namespace creation searched through all candidate
> @@ -2301,6 +2298,10 @@ static int do_xaction_namespace(const char *namespace,
> if (saved_rc)
> rc = saved_rc;
>
> +out:
> + if (ri_ctx.f_out && ri_ctx.f_out != stdout)
> + fclose(ri_ctx.f_out);
> +
> return rc;
> }
>
> --
> 2.23.0
>
>
>
>
> .
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace()
2021-06-30 17:47 ` Alison Schofield
@ 2021-07-01 1:42 ` Zhiqiang Liu
0 siblings, 0 replies; 9+ messages in thread
From: Zhiqiang Liu @ 2021-07-01 1:42 UTC (permalink / raw)
To: Alison Schofield; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng
On 2021/7/1 1:47, Alison Schofield wrote:
> On Tue, Jun 15, 2021 at 08:39:20PM +0800, Zhiqiang Liu wrote:
>> In do_xaction_namespace(), ri_ctx.f_out should be closed after
>> being opened.
>>
> Hi Zhiqiang,
>
> The commit message and commit log need to be swapped.
>
> Something like:
>
> Commit message says what the patch does:
> [ndctl PATCH 2/2] namespace: Close fd before return in do_xaction_namespace()
>
> Commit log says why it needs to be done:
> This prevents a potential file descriptor leak in do_xaction_namespace()
>
> And, same as in Patch 1 - mention it was found by Coverity.
>
> Alison
Thanks for your advice.
I will do that as your suggestion in v2 patch.
>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> ---
>> ndctl/namespace.c | 17 +++++++++--------
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
>> index 21089d7..55364ac 100644
>> --- a/ndctl/namespace.c
>> +++ b/ndctl/namespace.c
>> @@ -2141,7 +2141,7 @@ static int do_xaction_namespace(const char *namespace,
>> util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
>> if (rc >= 0)
>> (*processed)++;
>> - return rc;
>> + goto out;
>> }
>> }
>>
>> @@ -2152,11 +2152,11 @@ static int do_xaction_namespace(const char *namespace,
>> rc = file_write_infoblock(param.outfile);
>> if (rc >= 0)
>> (*processed)++;
>> - return rc;
>> + goto out;
>> }
>>
>> if (!namespace && action != ACTION_CREATE)
>> - return rc;
>> + goto out;
>>
>> if (verbose)
>> ndctl_set_log_priority(ctx, LOG_DEBUG);
>> @@ -2212,7 +2212,7 @@ static int do_xaction_namespace(const char *namespace,
>> saved_rc = rc;
>> continue;
>> }
>> - return rc;
>> + goto out;
>> }
>> ndctl_namespace_foreach_safe(region, ndns, _n) {
>> ndns_name = ndctl_namespace_get_devname(ndns);
>> @@ -2259,7 +2259,7 @@ static int do_xaction_namespace(const char *namespace,
>> rc = namespace_reconfig(region, ndns);
>> if (rc == 0)
>> *processed = 1;
>> - return rc;
>> + goto out;
>> case ACTION_READ_INFOBLOCK:
>> rc = namespace_rw_infoblock(ndns, &ri_ctx, READ);
>> if (rc == 0)
>> @@ -2281,9 +2281,6 @@ static int do_xaction_namespace(const char *namespace,
>> if (ri_ctx.jblocks)
>> util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
>>
>> - if (ri_ctx.f_out && ri_ctx.f_out != stdout)
>> - fclose(ri_ctx.f_out);
>> -
>> if (action == ACTION_CREATE && rc == -EAGAIN) {
>> /*
>> * Namespace creation searched through all candidate
>> @@ -2301,6 +2298,10 @@ static int do_xaction_namespace(const char *namespace,
>> if (saved_rc)
>> rc = saved_rc;
>>
>> +out:
>> + if (ri_ctx.f_out && ri_ctx.f_out != stdout)
>> + fclose(ri_ctx.f_out);
>> +
>> return rc;
>> }
>>
>> --
>> 2.23.0
>>
>>
>>
>>
>> .
>>
>>
>>
> .
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace
2021-06-30 17:41 ` Alison Schofield
@ 2021-07-01 3:23 ` Zhiqiang Liu
2021-07-06 4:35 ` Zhiqiang Liu
0 siblings, 1 reply; 9+ messages in thread
From: Zhiqiang Liu @ 2021-07-01 3:23 UTC (permalink / raw)
To: Alison Schofield; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng
On 2021/7/1 1:41, Alison Schofield wrote:
> On Tue, Jun 15, 2021 at 08:38:33PM +0800, Zhiqiang Liu wrote:
>> ndctl_pfn_get_namespace() may return NULL, so callers
>> should check return value of it. Otherwise, it may
>> cause access NULL pointer problem.
>>
> Hi Zhiqiang,
>
> I see you mentioned this was found by Coverity in the cover letter.
> Please repeat that in the commit log here.
>
> What about the call path:
> ndctl_dax_get_namespace() --> ndctl_pfn_get_namespace()
>
> Seems like another place where ndctl_pfn_get_namespace() could
> eventually lead to a NULL ptr dereference.
>
> Alison
Thanks for your reply.
Call path:
namespace_clear_bb
-> if (dax) dax_clear_badblocks(dax)
->ndctl_dax_get_namespace(dax)
->ndctl_pfn_get_namespace(&dax->pfn)
->ndctl_pfn_get_ctx(pfn)
struct ndctl_dax {
struct ndctl_pfn pfn;
struct daxctl_region *region;
};
Here, we have checked that dax is not NULL before calling ndctl_dax_get_namespace(dax).
If dax is not a NULL pointer, the dax->pfn will not be a NULL pointer.
As for ndctl_pfn_get_ctx(pfn), it will access pfn->region->bus without NULL-checking, which
may lead to a NULL ptr dereference as you said. I will correct it in the v2 patch.
Regards,
Zhiqiang Liu
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> ---
>> ndctl/namespace.c | 18 ++++++++++++++----
>> test/libndctl.c | 4 ++--
>> util/json.c | 2 ++
>> 3 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
>> index 0c8df9f..21089d7 100644
>> --- a/ndctl/namespace.c
>> +++ b/ndctl/namespace.c
>> @@ -1417,11 +1417,16 @@ static int nstype_clear_badblocks(struct ndctl_namespace *ndns,
>>
>> static int dax_clear_badblocks(struct ndctl_dax *dax)
>> {
>> - struct ndctl_namespace *ndns = ndctl_dax_get_namespace(dax);
>> - const char *devname = ndctl_dax_get_devname(dax);
>> + struct ndctl_namespace *ndns;
>> + const char *devname;
>> unsigned long long begin, size;
>> int rc;
>>
>> + ndns = ndctl_dax_get_namespace(dax);
>> + if (!ndns)
>> + return -ENXIO;
>> +
>> + devname = ndctl_dax_get_devname(dax);
>> begin = ndctl_dax_get_resource(dax);
>> if (begin == ULLONG_MAX)
>> return -ENXIO;
>> @@ -1441,11 +1446,16 @@ static int dax_clear_badblocks(struct ndctl_dax *dax)
>>
>> static int pfn_clear_badblocks(struct ndctl_pfn *pfn)
>> {
>> - struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
>> - const char *devname = ndctl_pfn_get_devname(pfn);
>> + struct ndctl_namespace *ndns;
>> + const char *devname;
>> unsigned long long begin, size;
>> int rc;
>>
>> + ndns = ndctl_pfn_get_namespace(pfn);
>> + if (!ndns)
>> + return -ENXIO;
>> +
>> + devname = ndctl_pfn_get_devname(pfn);
>> begin = ndctl_pfn_get_resource(pfn);
>> if (begin == ULLONG_MAX)
>> return -ENXIO;
>> diff --git a/test/libndctl.c b/test/libndctl.c
>> index 24d72b3..05e5ff2 100644
>> --- a/test/libndctl.c
>> +++ b/test/libndctl.c
>> @@ -1275,7 +1275,7 @@ static int check_pfn_autodetect(struct ndctl_bus *bus,
>> if (!ndctl_pfn_is_enabled(pfn))
>> continue;
>> pfn_ndns = ndctl_pfn_get_namespace(pfn);
>> - if (strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
>> + if (!pfn_ndns || strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
>> continue;
>> fprintf(stderr, "%s: pfn_ndns: %p ndns: %p\n", __func__,
>> pfn_ndns, ndns);
>> @@ -1372,7 +1372,7 @@ static int check_dax_autodetect(struct ndctl_bus *bus,
>> if (!ndctl_dax_is_enabled(dax))
>> continue;
>> dax_ndns = ndctl_dax_get_namespace(dax);
>> - if (strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
>> + if (!dax_ndns || strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
>> continue;
>> fprintf(stderr, "%s: dax_ndns: %p ndns: %p\n", __func__,
>> dax_ndns, ndns);
>> diff --git a/util/json.c b/util/json.c
>> index ca0167b..249f021 100644
>> --- a/util/json.c
>> +++ b/util/json.c
>> @@ -1002,6 +1002,8 @@ static struct json_object *util_pfn_badblocks_to_json(struct ndctl_pfn *pfn,
>> pfn_begin = ndctl_pfn_get_resource(pfn);
>> if (pfn_begin == ULLONG_MAX) {
>> struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
>> + if (!ndns)
>> + return NULL;
>>
>> return util_namespace_badblocks_to_json(ndns, bb_count, flags);
>> }
>> --
>> 2.23.0
>>
>>
>>
>>
>> .
>>
>>
>>
> .
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace
2021-07-01 3:23 ` Zhiqiang Liu
@ 2021-07-06 4:35 ` Zhiqiang Liu
0 siblings, 0 replies; 9+ messages in thread
From: Zhiqiang Liu @ 2021-07-06 4:35 UTC (permalink / raw)
To: Alison Schofield; +Cc: vishal.l.verma, nvdimm, linfeilong, lixiaokeng
On 2021/7/1 11:23, Zhiqiang Liu wrote:
>
> On 2021/7/1 1:41, Alison Schofield wrote:
>> On Tue, Jun 15, 2021 at 08:38:33PM +0800, Zhiqiang Liu wrote:
>>> ndctl_pfn_get_namespace() may return NULL, so callers
>>> should check return value of it. Otherwise, it may
>>> cause access NULL pointer problem.
>>>
>> Hi Zhiqiang,
>>
>> I see you mentioned this was found by Coverity in the cover letter.
>> Please repeat that in the commit log here.
>>
>> What about the call path:
>> ndctl_dax_get_namespace() --> ndctl_pfn_get_namespace()
>>
>> Seems like another place where ndctl_pfn_get_namespace() could
>> eventually lead to a NULL ptr dereference.
>>
>> Alison
>
> Thanks for your reply.
>
> Call path:
>
> namespace_clear_bb
>
> -> if (dax) dax_clear_badblocks(dax)
>
> ->ndctl_dax_get_namespace(dax)
>
> ->ndctl_pfn_get_namespace(&dax->pfn)
>
> ->ndctl_pfn_get_ctx(pfn)
>
>
> struct ndctl_dax {
> struct ndctl_pfn pfn;
> struct daxctl_region *region;
> };
>
> Here, we have checked that dax is not NULL before calling ndctl_dax_get_namespace(dax).
>
> If dax is not a NULL pointer, the dax->pfn will not be a NULL pointer.
>
> As for ndctl_pfn_get_ctx(pfn), it will access pfn->region->bus without NULL-checking, which
>
> may lead to a NULL ptr dereference as you said. I will correct it in the v2 patch.
>
Null_check in ndctl_pfn_get_**(pfn) is a common problem. I will try to solve this kind
problem in new patches.
Regards
Zhiqiang Liu
>
> Regards,
>
> Zhiqiang Liu
>
>>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>> ---
>>> ndctl/namespace.c | 18 ++++++++++++++----
>>> test/libndctl.c | 4 ++--
>>> util/json.c | 2 ++
>>> 3 files changed, 18 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
>>> index 0c8df9f..21089d7 100644
>>> --- a/ndctl/namespace.c
>>> +++ b/ndctl/namespace.c
>>> @@ -1417,11 +1417,16 @@ static int nstype_clear_badblocks(struct ndctl_namespace *ndns,
>>>
>>> static int dax_clear_badblocks(struct ndctl_dax *dax)
>>> {
>>> - struct ndctl_namespace *ndns = ndctl_dax_get_namespace(dax);
>>> - const char *devname = ndctl_dax_get_devname(dax);
>>> + struct ndctl_namespace *ndns;
>>> + const char *devname;
>>> unsigned long long begin, size;
>>> int rc;
>>>
>>> + ndns = ndctl_dax_get_namespace(dax);
>>> + if (!ndns)
>>> + return -ENXIO;
>>> +
>>> + devname = ndctl_dax_get_devname(dax);
>>> begin = ndctl_dax_get_resource(dax);
>>> if (begin == ULLONG_MAX)
>>> return -ENXIO;
>>> @@ -1441,11 +1446,16 @@ static int dax_clear_badblocks(struct ndctl_dax *dax)
>>>
>>> static int pfn_clear_badblocks(struct ndctl_pfn *pfn)
>>> {
>>> - struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
>>> - const char *devname = ndctl_pfn_get_devname(pfn);
>>> + struct ndctl_namespace *ndns;
>>> + const char *devname;
>>> unsigned long long begin, size;
>>> int rc;
>>>
>>> + ndns = ndctl_pfn_get_namespace(pfn);
>>> + if (!ndns)
>>> + return -ENXIO;
>>> +
>>> + devname = ndctl_pfn_get_devname(pfn);
>>> begin = ndctl_pfn_get_resource(pfn);
>>> if (begin == ULLONG_MAX)
>>> return -ENXIO;
>>> diff --git a/test/libndctl.c b/test/libndctl.c
>>> index 24d72b3..05e5ff2 100644
>>> --- a/test/libndctl.c
>>> +++ b/test/libndctl.c
>>> @@ -1275,7 +1275,7 @@ static int check_pfn_autodetect(struct ndctl_bus *bus,
>>> if (!ndctl_pfn_is_enabled(pfn))
>>> continue;
>>> pfn_ndns = ndctl_pfn_get_namespace(pfn);
>>> - if (strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
>>> + if (!pfn_ndns || strcmp(ndctl_namespace_get_devname(pfn_ndns), devname) != 0)
>>> continue;
>>> fprintf(stderr, "%s: pfn_ndns: %p ndns: %p\n", __func__,
>>> pfn_ndns, ndns);
>>> @@ -1372,7 +1372,7 @@ static int check_dax_autodetect(struct ndctl_bus *bus,
>>> if (!ndctl_dax_is_enabled(dax))
>>> continue;
>>> dax_ndns = ndctl_dax_get_namespace(dax);
>>> - if (strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
>>> + if (!dax_ndns || strcmp(ndctl_namespace_get_devname(dax_ndns), devname) != 0)
>>> continue;
>>> fprintf(stderr, "%s: dax_ndns: %p ndns: %p\n", __func__,
>>> dax_ndns, ndns);
>>> diff --git a/util/json.c b/util/json.c
>>> index ca0167b..249f021 100644
>>> --- a/util/json.c
>>> +++ b/util/json.c
>>> @@ -1002,6 +1002,8 @@ static struct json_object *util_pfn_badblocks_to_json(struct ndctl_pfn *pfn,
>>> pfn_begin = ndctl_pfn_get_resource(pfn);
>>> if (pfn_begin == ULLONG_MAX) {
>>> struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
>>> + if (!ndns)
>>> + return NULL;
>>>
>>> return util_namespace_badblocks_to_json(ndns, bb_count, flags);
>>> }
>>> --
>>> 2.23.0
>>>
>>>
>>>
>>>
>>> .
>>>
>>>
>>>
>> .
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-06 4:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15 12:37 [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu
2021-06-15 12:38 ` [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace Zhiqiang Liu
2021-06-30 17:41 ` Alison Schofield
2021-07-01 3:23 ` Zhiqiang Liu
2021-07-06 4:35 ` Zhiqiang Liu
2021-06-15 12:39 ` [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace() Zhiqiang Liu
2021-06-30 17:47 ` Alison Schofield
2021-07-01 1:42 ` Zhiqiang Liu
2021-06-30 2:55 ` [ndctl PATCH 0/2] fix two issues reported by Coverity Zhiqiang Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).