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 <[email protected]>
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
