[PATCH 0/2] cxl/region: Fix a couple memory safety bugs

[PATCH 0/2] cxl/region: Fix a couple memory safety bugs

[Dan Williams]

memdev_filter_pos() has 2 issues surfaced by unit testing, a leak, and
incorrect sort results.

---

Dan Williams (2):
      cxl/region: Fix memdev_filter_pos() memory leak
      cxl/region: Fix memdev_filter_pos() separator detection


 cxl/region.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

[PATCH 1/2] cxl/region: Fix memdev_filter_pos() memory leak

[Dan Williams]

Arrange to free the temporary duplicate of @_csv in all exit paths. Just
share a common exit path.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/region.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cxl/region.c b/cxl/region.c
index 15cac64a158c..2202afa440ef 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -183,7 +183,7 @@ static int memdev_filter_pos(struct json_object *jobj, const char *_csv)
 	for (pos = 0, arg = strtok_r(csv, which_sep(csv), &save); arg;
 	     arg = strtok_r(NULL, which_sep(csv), &save), pos++)
 		if (util_cxl_memdev_filter(memdev, arg, NULL))
-			return pos;
+			break;
 	free(csv);
 
 	return pos;

Re: [PATCH 1/2] cxl/region: Fix memdev_filter_pos() memory leak

[Alison Schofield]

On Wed, Dec 14, 2022 at 05:33:52PM -0800, Dan Williams wrote:
> Arrange to free the temporary duplicate of @_csv in all exit paths. Just
> share a common exit path.
> 

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/region.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 15cac64a158c..2202afa440ef 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -183,7 +183,7 @@ static int memdev_filter_pos(struct json_object *jobj, const char *_csv)
>  	for (pos = 0, arg = strtok_r(csv, which_sep(csv), &save); arg;
>  	     arg = strtok_r(NULL, which_sep(csv), &save), pos++)
>  		if (util_cxl_memdev_filter(memdev, arg, NULL))
> -			return pos;
> +			break;
>  	free(csv);
>  
>  	return pos;
> 

[PATCH 2/2] cxl/region: Fix memdev_filter_pos() separator detection

[Dan Williams]

Stop using the modified version of the @_csv as the string to determine
the separator it may cause which_sep() to get different answers each
iteration. Moreover there is no need to retrieve the separator each
iteration, so just retrieve it once from @_csv.

If the separator is mismatched it can lead to the sort not following the
specified device order for create-region.

Reported-by: Luis Chamberlain <mcgrof@kernel.org>
Link: https://gist.github.com/mcgrof/aeac639365a651bd77f143cf38eb7493
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/region.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cxl/region.c b/cxl/region.c
index 2202afa440ef..bb3a10a7713c 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -172,6 +172,7 @@ static struct sort_context {
 static int memdev_filter_pos(struct json_object *jobj, const char *_csv)
 {
 	struct cxl_memdev *memdev = json_object_get_userdata(jobj);
+	const char *sep = which_sep(_csv);
 	char *csv, *save;
 	const char *arg;
 	int pos;
@@ -180,8 +181,8 @@ static int memdev_filter_pos(struct json_object *jobj, const char *_csv)
 	if (!csv)
 		return -1;
 
-	for (pos = 0, arg = strtok_r(csv, which_sep(csv), &save); arg;
-	     arg = strtok_r(NULL, which_sep(csv), &save), pos++)
+	for (pos = 0, arg = strtok_r(csv, sep, &save); arg;
+	     arg = strtok_r(NULL, sep, &save), pos++)
 		if (util_cxl_memdev_filter(memdev, arg, NULL))
 			break;
 	free(csv);

Re: [PATCH 2/2] cxl/region: Fix memdev_filter_pos() separator detection

[Alison Schofield]

On Wed, Dec 14, 2022 at 05:33:57PM -0800, Dan Williams wrote:
> Stop using the modified version of the @_csv as the string to determine
> the separator it may cause which_sep() to get different answers each
> iteration. Moreover there is no need to retrieve the separator each
> iteration, so just retrieve it once from @_csv.
> 
> If the separator is mismatched it can lead to the sort not following the
> specified device order for create-region.
> 

Tested-by: Alison Schofield <alison.schofield@intel.com>

> Reported-by: Luis Chamberlain <mcgrof@kernel.org>
> Link: https://gist.github.com/mcgrof/aeac639365a651bd77f143cf38eb7493
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/region.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 2202afa440ef..bb3a10a7713c 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -172,6 +172,7 @@ static struct sort_context {
>  static int memdev_filter_pos(struct json_object *jobj, const char *_csv)
>  {
>  	struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> +	const char *sep = which_sep(_csv);
>  	char *csv, *save;
>  	const char *arg;
>  	int pos;
> @@ -180,8 +181,8 @@ static int memdev_filter_pos(struct json_object *jobj, const char *_csv)
>  	if (!csv)
>  		return -1;
>  
> -	for (pos = 0, arg = strtok_r(csv, which_sep(csv), &save); arg;
> -	     arg = strtok_r(NULL, which_sep(csv), &save), pos++)
> +	for (pos = 0, arg = strtok_r(csv, sep, &save); arg;
> +	     arg = strtok_r(NULL, sep, &save), pos++)
>  		if (util_cxl_memdev_filter(memdev, arg, NULL))
>  			break;
>  	free(csv);
>