[PATCH] tools/rv: harden monitor name lookup bounds checks

[PATCH] tools/rv: harden monitor name lookup bounds checks

[unknownbbqrx]

Bound monitor-name derived copies in __ikm_find_monitor_name() and avoid unbounded writes from sprintf()/memcpy().

Pass the output buffer size from the caller, validate extracted line length from rv/available_monitors, and use snprintf() with truncation checks when building container monitor names.

Signed-off-by: unknownbbqrx <dev@unknownbbqr.xyz>
---
 tools/verification/rv/src/in_kernel.c | 34 +++++++++++++++++++++------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/tools/verification/rv/src/in_kernel.c b/tools/verification/rv/src/in_kernel.c
index d32453824..f17eac9b6 100644
--- a/tools/verification/rv/src/in_kernel.c
+++ b/tools/verification/rv/src/in_kernel.c
@@ -56,9 +56,12 @@ static int __ikm_read_enable(char *monitor_name)
  * The string out_name is populated with the full name, which can be
  * equal to monitor_name or container/monitor_name if nested
  */
-static int __ikm_find_monitor_name(char *monitor_name, char *out_name)
+static int __ikm_find_monitor_name(char *monitor_name, char *out_name,
+				  size_t out_name_size)
 {
-	char *available_monitors, container[MAX_DA_NAME_LEN+1], *cursor, *end;
+	char *available_monitors, container[MAX_DA_NAME_LEN + 2], *cursor, *end;
+	size_t len;
+	int n;
 	int retval = 1;
 
 	available_monitors = tracefs_instance_file_read(NULL, "rv/available_monitors", NULL);
@@ -72,17 +75,34 @@ static int __ikm_find_monitor_name(char *monitor_name, char *out_name)
 	}
 
 	for (; cursor > available_monitors; cursor--)
-		if (*(cursor-1) == '\n')
+		if (*(cursor - 1) == '\n')
 			break;
+
 	end = strstr(cursor, "\n");
-	memcpy(out_name, cursor, end-cursor);
-	out_name[end-cursor] = '\0';
+	if (!end) {
+		retval = -1;
+		goto out_free;
+	}
+
+	len = end - cursor;
+	if (len >= out_name_size) {
+		retval = -1;
+		goto out_free;
+	}
+
+	memcpy(out_name, cursor, len);
+	out_name[len] = '\0';
 
 	cursor = strstr(out_name, ":");
 	if (cursor)
 		*cursor = '/';
 	else {
-		sprintf(container, "%s:", monitor_name);
+		n = snprintf(container, sizeof(container), "%s:", monitor_name);
+		if (n < 0 || (size_t)n >= sizeof(container)) {
+			retval = -1;
+			goto out_free;
+		}
+
 		if (strstr(available_monitors, container))
 			config_is_container = 1;
 	}
@@ -782,7 +802,7 @@ int ikm_run_monitor(char *monitor_name, int argc, char **argv)
 	else
 		nested_name = monitor_name;
 
-	retval = __ikm_find_monitor_name(monitor_name, full_name);
+	retval = __ikm_find_monitor_name(monitor_name, full_name, sizeof(full_name));
 	if (!retval)
 		return 0;
 	if (retval < 0) {

base-commit: 2e68039281932e6dc37718a1ea7cbb8e2cda42e6
prerequisite-patch-id: b61dd51dee390277603975bf729a687113185c3a
-- 
2.53.0

Re: [PATCH] tools/rv: harden monitor name lookup bounds checks

[Gabriele Monaco]

On Thu, 2026-04-23 at 17:44 +0300, unknownbbqrx wrote:
> 
> Bound monitor-name derived copies in __ikm_find_monitor_name() and
> avoid unbounded writes from sprintf()/memcpy().
> 
> Pass the output buffer size from the caller, validate extracted line
> length from rv/available_monitors, and use snprintf() with truncation
> checks when building container monitor names.
> 
> Signed-off-by: unknownbbqrx <dev@unknownbbqr.xyz>

Hi,

thanks for the fix, however __ikm_find_monitor_name() is already a bit
sloppy (strstr can take any substring as a valid monitor) so I have a
patch to refactor it, which I'm about to send.
This will make your fix obsolete so I'm likely not going to take this
patch.

Thanks anyway,
Gabriele

> ---
>  tools/verification/rv/src/in_kernel.c | 34 +++++++++++++++++++++----
> --
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/verification/rv/src/in_kernel.c
> b/tools/verification/rv/src/in_kernel.c
> index d32453824..f17eac9b6 100644
> --- a/tools/verification/rv/src/in_kernel.c
> +++ b/tools/verification/rv/src/in_kernel.c
> @@ -56,9 +56,12 @@ static int __ikm_read_enable(char *monitor_name)
>   * The string out_name is populated with the full name, which can be
>   * equal to monitor_name or container/monitor_name if nested
>   */
> -static int __ikm_find_monitor_name(char *monitor_name, char
> *out_name)
> +static int __ikm_find_monitor_name(char *monitor_name, char
> *out_name,
> +				  size_t out_name_size)
>  {
> -	char *available_monitors, container[MAX_DA_NAME_LEN+1],
> *cursor, *end;
> +	char *available_monitors, container[MAX_DA_NAME_LEN + 2],
> *cursor, *end;
> +	size_t len;
> +	int n;
>  	int retval = 1;
>  
>  	available_monitors = tracefs_instance_file_read(NULL,
> "rv/available_monitors", NULL);
> @@ -72,17 +75,34 @@ static int __ikm_find_monitor_name(char
> *monitor_name, char *out_name)
>  	}
>  
>  	for (; cursor > available_monitors; cursor--)
> -		if (*(cursor-1) == '\n')
> +		if (*(cursor - 1) == '\n')
>  			break;
> +
>  	end = strstr(cursor, "\n");
> -	memcpy(out_name, cursor, end-cursor);
> -	out_name[end-cursor] = '\0';
> +	if (!end) {
> +		retval = -1;
> +		goto out_free;
> +	}
> +
> +	len = end - cursor;
> +	if (len >= out_name_size) {
> +		retval = -1;
> +		goto out_free;
> +	}
> +
> +	memcpy(out_name, cursor, len);
> +	out_name[len] = '\0';
>  
>  	cursor = strstr(out_name, ":");
>  	if (cursor)
>  		*cursor = '/';
>  	else {
> -		sprintf(container, "%s:", monitor_name);
> +		n = snprintf(container, sizeof(container), "%s:",
> monitor_name);
> +		if (n < 0 || (size_t)n >= sizeof(container)) {
> +			retval = -1;
> +			goto out_free;
> +		}
> +
>  		if (strstr(available_monitors, container))
>  			config_is_container = 1;
>  	}
> @@ -782,7 +802,7 @@ int ikm_run_monitor(char *monitor_name, int argc,
> char **argv)
>  	else
>  		nested_name = monitor_name;
>  
> -	retval = __ikm_find_monitor_name(monitor_name, full_name);
> +	retval = __ikm_find_monitor_name(monitor_name, full_name,
> sizeof(full_name));
>  	if (!retval)
>  		return 0;
>  	if (retval < 0) {
> 
> base-commit: 2e68039281932e6dc37718a1ea7cbb8e2cda42e6
> prerequisite-patch-id: b61dd51dee390277603975bf729a687113185c3a