[ndctl PATCH 0/2] fix two issues reported by Coverity

[ndctl PATCH 0/2] fix two issues reported by Coverity

[Zhiqiang Liu]

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



.

[ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace

[Zhiqiang Liu]

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




.

[ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace()

[Zhiqiang Liu]

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




.

Re: [ndctl PATCH 0/2] fix two issues reported by Coverity

[Zhiqiang Liu]

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(-)
>

Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace

[Alison Schofield]

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
> 
> 
> 
> 
> .
> 
> 
> 

Re: [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace()

[Alison Schofield]

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
> 
> 
> 
> 
> .
> 
> 
> 

Re: [ndctl PATCH 2/2] namespace: fix potentail fd leak problem in do_xaction_namespace()

[Zhiqiang Liu]

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
>>
>>
>>
>>
>> .
>>
>>
>>
> .

Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace

[Zhiqiang Liu]

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
>>
>>
>>
>>
>> .
>>
>>
>>
> .

Re: [ndctl PATCH 1/2] libndctl: check return value of ndctl_pfn_get_namespace

[Zhiqiang Liu]

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
>>>
>>>
>>>
>>>
>>> .
>>>
>>>
>>>
>> .