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

[Nick Alcock]

On 25 Mar 2022, Kris Van Hees outgrape:

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

Oh please. This gave me a nasty taste in my mouth (but it did work).

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

What a neat trick!

I'll give it a try.

-- 
NULL && (void)