[DTrace-devel] [PATCH] Fix length stored by strjoin() and optimise the implementation

[Kris Van Hees]

On Mon, Nov 15, 2021 at 05:26:27PM -0500, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> 
> You say "Fix length stored by strjoin()."  So the commit message should say
> what was wrong, and the patch should add a test to show that the problem was
> fixed.

Hm, ok, I figured it was rather obvious from the code, but ok, I can add a
short description of the problem.  We were storing strlen(s1) + strlen(s2) as
the length of the result which is obviously wrong since it can be > STRSZ.

No test needed because the tests caught this problem nicely and thus serve to
verify that it is fixed.

> Also,
> 
> On 11/15/21 12:32 PM, Kris Van Hees via DTrace-devel wrote:
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   bpf/strjoin.S | 63 +++++++++++++++++++++++++++++----------------------
> >   1 file changed, 36 insertions(+), 27 deletions(-)
> > 
> > diff --git a/bpf/strjoin.S b/bpf/strjoin.S
> > index da7afa3e..2f78695a 100644
> > --- a/bpf/strjoin.S
> > +++ b/bpf/strjoin.S
> > @@ -19,39 +19,48 @@ dt_strjoin:
> >   	mov	%r7, %r2		/* %r7 = s1 */
> >   	mov	%r8, %r3		/* %r8 = s2 */
> > -	mov	%r1, %r7
> > -	call	dt_strlen
> > -	mov	%r6, %r0		/* len = dt_strlen(s1) */
> > -	add	%r7, DT_STRLEN_BYTES	/* s1 += 2 */
> > -
> > -	mov	%r1, %r8
> > -	call	dt_strlen
> > -	add	%r6, %r0		/* len += dt_strlen(s2) */
> > -	add	%r8, DT_STRLEN_BYTES	/* s2 += 2 */
> > -
> > -	mov	%r1, %r6
> > -	mov	%r2, %r9
> > -	call	dt_strlen_store		/* dt_strlen_store(len, dst) */
> > -	add	%r9, DT_STRLEN_BYTES	/* dst += 2 */
> > -
> >   	lddw	%r6, STRSZ
> > -	add	%r6, 1
> > -	and	%r6, 0xffffffff		/* len = (STRSZ + 1) & 0xffffffff */
> > +	add	%r6, 1			/* cnt = STRSZ + 1 */
> > -	mov	%r1, %r9
> > +	add	%r1, DT_STRLEN_BYTES	/* dst is in %r1 already */
> >   	mov	%r2, %r6
> >   	mov	%r3, %r7
> > -	call	BPF_FUNC_probe_read_str	/* cnt = bpf_probe_read_str(dst, len s1) */
> > -	jslt	%r0, 0, .L1		/* if (cnt < 0) goto .L1 */
> > -	jslt	%r0, 1, .L2		/* if (cnt < 1) goto .L2 */
> > -	sub	%r0, 1			/* cnt-- */
> > -	add	%r9, %r0		/* dst += cnt */
> > -	sub	%r6, %r0		/* len -= cnt */
> > -.L2:
> > +	add	%r3, DT_STRLEN_BYTES
> > +	call	BPF_FUNC_probe_read_str	/*
> > +					 * rc = probe_read_str(
> > +					 *	  &dst[DT_STRLEN_BYTES],
> > +					 *	  cnt,
> > +					 *	  &s1[DT_STRLEN_BYTES]);
> > +					 */
> > +	jslt	%r0, 0, .Lexit		/* if (rc < 0) goto .Lexit; */
> > +	mov	%r7, %r0		/* len = rc */
> > +	jslt	%r7, 1, .Ls2_only	/* if (len < 1) goto .Ls2_only; */
> 
> After the probe_read_str() call, the return value cannot be 0. Either it is
> negative (error) or it is strictly positive.  So, the first test might as
> well be "jsle".  Then the second test (and the Ls2_only label) are no longer
> needed.

I disagree with the jsle (which is why I didn't use it) because the jslt is
more in line with the documented return values.  Negative indicates error, so
we jump when it is negative.  Then again, it seems that the verifier gets a
bit confused because it thinks probe_read_str() *can* return 0, and this it
ends up with a umin_value of -1 later on...  OK, so jsle it is.  Bah!

Then the 2nd jslt can indeed go away (and the label).

> Now, let's say probe_read_str() errors out.  Then we silently return
> garbage?  Are we happy with that?  (Maybe we are.  I don't know.)

Well, that is exactly the same behaviour as before so in that sense the patch
is consistent and not overreaching.  But I am investigating the best way to
handle issues like that (and ways to simulate them nicely so we can add tests
for it).  It will probably have to wait until we have the code ti retrieve
strings from arbitrary kernel and user addresses, which will then make it
possible to create valid tests for those conditions.

> For comments regarding instructions with signed conditionals, might as well
> use (e.g.) "s<", "s<=", and the like.

Sure.

> > +	sub	%r7, 1			/* len-- */
> > +	jsge	%r7, %r6, .Lset_len	/* if (len >= cnt) goto .Lset_len; */
> 
> Actually, at this stage, we know len<cnt.  Even the BPF verifier knows
> this.  So the test and the Lset_len label can be dropped.

Well, no, because this is another bug.  We should test for len == cnt and if
so we just because we have no space left for the 2nd string.

So this all becomes:

        jsle    %r0, 0, .Lexit          /* if (rc s<= 0) goto .Lexit; */
        mov     %r7, %r0                /* len = rc */
        jeq     %r7, %r6, .Lset_len     /* if (len == cnt) goto .Lset_len; */
        sub     %r7, 1                  /* len-- */

> > +
> > +.Ls2_only:
> >   	mov	%r1, %r9
> > +	add	%r1, DT_STRLEN_BYTES
> > +	add	%r1, %r7
> >   	mov	%r2, %r6
> > +	sub	%r2, %r7
> >   	mov	%r3, %r8
> > -	call	BPF_FUNC_probe_read_str	/* bpf_probe_read_str(dst, len, s2 */
> > -.L1:
> > +	add	%r3, DT_STRLEN_BYTES
> > +	call	BPF_FUNC_probe_read_str	/*
> > +					 * rc = probe_read_str(
> > +					 *	  &dst[DT_STRLEN_BYTES + len],
> > +					 *	  cnt - len,
> > +					 *	  &s2[DT_STRLEN_BYTES]);
> > +					 */
> > +	jslt	%r0, 0, .Lexit		/* if (rc < 0) goto .Lset_len; */
> > +	add	%r7, %r0
> > +	sub	%r7, 1			/* len += rc - 1 */
> > +
> > +.Lset_len:
> > +	mov	%r1, %r7
> > +	mov	%r2, %r9
> > +	call	dt_strlen_store		/* dt_strlen_store(len - 1, dst) */
> > +
> > +.Lexit:
> >   	exit
> >   	.size	dt_strjoin, .-dt_strjoin
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel