[DTrace-devel] [PATCH v3 18/19] strings: improve bounds on strlen return value

[Kris Van Hees]

On Thu, Mar 24, 2022 at 06:24:44PM +0000, Nick Alcock via DTrace-devel wrote:
> Checking for a positive bound rather than a zero one prevents the
> minimum range of the return value of strlen coming out of the verifier
> as -1 (which can cause later problems if the verifier decides it needs a
> non-negative arg).

I propose a different solution (see below) to avoid the confusion of setting
ken to be 1 just to then subtract 1 from it.

> We know the return value from bpf_probe_read_str must be non-negative
> anyway, as the comment in the following line notes: this just teaches
> this to the verifier.
> 
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>  bpf/strlen.c      |  2 +-
>  include/bpf-lib.h | 15 +++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/bpf/strlen.c b/bpf/strlen.c
> index 71ddbca42f01..e10070c9bdaf 100644
> --- a/bpf/strlen.c
> +++ b/bpf/strlen.c
> @@ -21,7 +21,7 @@ noinline uint64_t dt_strlen(const dt_dctx_t *dctx, const char *str)
>  	int64_t	len;
>  
>  	len = bpf_probe_read_str(tmp, (uint64_t)&STRSZ + 1, str);
> -	set_not_neg_bound(len);
> +	set_positive_bound(len);
>  
>  	return len - 1;		/* bpf_probe_read_str() never returns 0 */

Instead do:

	len = bpf_probe_read_str(tmp, (uint64_t)&STRSZ + 1, str) - 1;
	set_not_neg_bound(len);

	return len;

That accomplishes the same thing and it is valid to subtract 1 from the return
value of bpf_probe_read_str) because that will make an error return code more
negative (and we will map that to a 0 length), or it will give us the correct
string length because bpf_probe_read_str() return value includes the terminating
0-byte.

>  }
> diff --git a/include/bpf-lib.h b/include/bpf-lib.h
> index d7078da5c146..1c0d4a0c15cd 100644
> --- a/include/bpf-lib.h
> +++ b/include/bpf-lib.h
> @@ -61,5 +61,20 @@
>                  : /* no clobbers */ \
>          );
>  
> +/*
> + * Explicit inline assembler to implement a positive bound check:
> + *
> + *	if (var < 1)
> + *		var = 1;
> + */
> +#define set_positive_bound(var) \
> +	asm ("jsgt %0, 0, 1f\n\t" \
> +             "mov %0, 1\n\t" \
> +             "1:" \
> +                : "+r" (var) \
> +                : /* no inputs */ \
> +                : /* no clobbers */ \
> +        );
> +

Not needed with the alternative above.

>  #endif /* BPF_LIB_H */
> -- 
> 2.35.1.261.g8402f930ba.dirty
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel