[DTrace-devel] [PATCH] cg: lift restriction of 1st arg to bcopy() not being alloca

[Kris Van Hees]

On Fri, Jan 06, 2023 at 02:29:09PM -0500, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> with comments below...
> 
> To start:
> 
> *)  Does this patch modify behavior of copyinto() as well?  If so, shouldn't
> that be mentioned and tested?

Yes and no.  But nothing that needs documenting and testing.  The copyinto()
subroutine copies from userspace addresses so those are never alloca pointers.
But no dtrace version has ever validated the source pointer for copyinto()
anyway because it truly can be an arbitrary pointer.

> *)  Modified files (including dt_cg.c) should have 2023 copyright.

Ah yes, forgot this was work from 2022 that I never pushed for review until
now.

> On 1/4/23 01:32, Kris Van Hees via DTrace-devel wrote:
> > The documentation states that the source address is not allowed to be
> > within the scratch memory region, but the legacy implementation does
> > not actually enforce that restriction, nor does it seem necessary.
> 
> I've lost track, but have we standardized on what "legacy" means in this
> context?  Will future developers know what this means?  Also, isn't Solaris
> the more compelling precedent?

We use it to refer to both the Solaris version and the pre-BPF versiobn, in
that they almost always implement the same semantics.  But I can update the
commit message to explicitly mention that dtrace has never enforced that
restriction.

> > This patch also moves bcopy tests into their own directory.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > @@ -4411,10 +4411,7 @@ dt_cg_subr_bcopy_impl(dt_node_t *dnp, dt_node_t *dst, dt_node_t *src,
> >   			"\t argument: non-alloca pointer\n",
> >   			is_bcopy ? "bcopy" : "copyinto", is_bcopy ? 2 : 3);
> >   	/* The dst will be NULL-checked in the alloca access check below. */
> > -
> > -	dt_cg_alloca_access_check(dlp, drp, dst->dn_reg,
> > -				  DT_ISREG, size->dn_reg);
> > -	dt_cg_alloca_ptr(dlp, drp, dst->dn_reg, dst->dn_reg);
> > +	dt_cg_check_ptr_arg(dlp, drp, dst, size);
> >   	if (dt_regset_xalloc_args(drp) == -1)
> >   		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> 
> Should that comment also be removed, since the alloca_access_check() call is
> removed?

No, but I will update it to make it clear that NULL and access checking is
doing in dt_cg_check_ptr_arg().

> > diff --git a/test/unittest/funcs/bcopy/err.badbcopy7.d b/test/unittest/funcs/bcopy/err.badbcopy7.d
> > new file mode 100644
> > index 00000000..c8fffea5
> > --- /dev/null
> > +++ b/test/unittest/funcs/bcopy/err.badbcopy7.d
> > @@ -0,0 +1,29 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
> 
> 2023?  And drop the 2006.  Whatever file this was cut and pasted from surely
> didn't contribute much to this test.  After all, as far as git is concerned,
> this is a new test.

Good point.

> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +/*
> > + * ASSERTION: bcopy from an alloca pointer is subject to size checks
> > + *
> > + * SECTION: Actions and Subroutines/alloca();
> > + * 	Actions and Subroutines/bcopy()
> > + *
> 
> Stylistic consistency would probably drop that blank comment line.

Hm, yes.  Though we have many tests that are still inconsistent - but doesn't
hurt to do it here since I believe we have been doing that for newer tests (or
updated tests).

> > + */
> > +
> > +#pragma D option quiet
> > +
> > +BEGIN
> > +{
> > +	d = alloca(20);
> > +	s = alloca(10);
> > +	bcopy(s, d, 20);
> 
> Not a big deal, but s and d might as well be src and dst.
> 
> > +	exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > +	exit(1);
> > +}
> > diff --git a/test/unittest/funcs/err.badbcopy8.d b/test/unittest/funcs/bcopy/err.badbcopy8.d
> > similarity index 100%
> > rename from test/unittest/funcs/err.badbcopy8.d
> > rename to test/unittest/funcs/bcopy/err.badbcopy8.d
> 
> Okay, so err.badbcopy8.d is being used "as is," but shouldn't it get some
> scrutiny?  It has:
>         ptr = alloca(20);
>         /* Attempt to bcopy to scratch memory that isn't allocated,
>            with a max exceeding the verifier-checked bound of
>            2*scratchsize.  */
>         bcopy((void *)&`max_pfn, ptr, 2048000);
> But the scratch memory clearly *IS* allocated.  If the point is the size is
> too large, then compared to what?  Apparently, larger than the allocated 20
> is just fine.  Maybe even up to scratchsize is fine.  Over 1*scratchsize is
> not okay, despite the comment.  And testing with 2048000 seems just plain
> irrelevant.

I am simply moving the bcopy tests to their own subdirectory.  I am not
validating the tests being moved within the context of this patch.  I would
suggest checking with Nick on whether this test is valid as-is or whether it
is not (or to provide a more clear explanation about it).

> > diff --git a/test/unittest/funcs/err.badbcopy7.d b/test/unittest/funcs/bcopy/tst.bcopy2.d
> > similarity index 65%
> > rename from test/unittest/funcs/err.badbcopy7.d
> > rename to test/unittest/funcs/bcopy/tst.bcopy2.d
> > index d7390122..6fa01d1c 100644
> > --- a/test/unittest/funcs/err.badbcopy7.d
> > +++ b/test/unittest/funcs/bcopy/tst.bcopy2.d
> > @@ -6,8 +6,8 @@
> >    */
> >   /*
> > - * ASSERTION:
> > - *	bcopy should not copy when the source is scratch space
> > + * ASSERTION: bcopy to scratch space is allowed (even though documentation
> > + *	      claims that it is not)
> >    *
> >    * SECTION: Actions and Subroutines/alloca();
> >    * 	Actions and Subroutines/bcopy()
> > @@ -16,14 +16,13 @@
> >   #pragma D option quiet
> > -
> >   BEGIN
> >   {
> > -	ptr = alloca(sizeof(unsigned long));
> > +	ptr = (unsigned long *)alloca(sizeof(unsigned long));
> >   	bcopy((void *)&`max_pfn, ptr, sizeof(unsigned long));
> > -	ptr2 = alloca(sizeof(unsigned long));
> > +	ptr2 = (unsigned long *)alloca(sizeof(unsigned long));
> >   	bcopy(ptr, ptr2, sizeof(unsigned long));
> > -	exit(0);
> > +	exit(*ptr == *ptr2 ? 0 : 1);
> >   }
> >   ERROR
> 
> I realize that this test is inherited from past work, but using
> bcopy(&`max_pfn,...) to initialize *ptr unnecessarily complicates this
> rather simple test.  Why not just *ptr=0x12345678, or something?  That makes
> clearer what's going on.  It'd also be clearer to use "src" and "dst" rather
> than "ptr" and "ptr2".

Overall, I agree, but I wanted to keep the original negative test as much as
possible to indicate that whereas the original test was verifying that using
an alloca pointer as source was not allowed, the new test verifies that it is
in fact allowed (and that it works).

	Kris