[DTrace-devel] [PATCH v3 06/21] dtprobed: add the DOF stash

[Kris Van Hees]

On Wed, Feb 14, 2024 at 02:54:17PM +0000, Nick Alcock wrote:
> On 14 Feb 2024, Kris Van Hees outgrape:
> 
> > On Mon, Feb 12, 2024 at 10:21:35AM -0500, Kris Van Hees via DTrace-devel wrote:
> >> > +/*
> >> > + * Free all the accumulated parsed DOF in the passed list.
> >> > + */
> >> > +void
> >> > +dof_stash_flush(dt_list_t *accum)
> >
> > That is a bit of an unusual function name for a function that frees a list.
> > When I see *flush I usually think of forcinfg things to be written out to
> > stable storage or something like that.  Maybe dof_stash_free() would be
> > better?
> 
> Yeah, that's a terrible name! Even the *comment* says what it should be
> called.
> 
> Process-wide things like this are usually named _init and _fini in
> DTrace, but in this case _init actually doesn't have an inverse (there
> is no _fini), and this function actually undoes a bunch of
> dof_stash_push_parsed(). This is what I thought _flush would be a good
> inverse of. Since that's actually a dreadful name I changed this to
> dof_stash_free().
> 
> (This causes minor modifications to the calls in a later commit, too.)
> 
> >> > +/*
> >> > + * Write out a piece of raw DOF.  Returns the length of the file written,
> >> > + * or 0 if none was needed (or -1 on error).
> >> > + */
> >> > +static int
> >> > +dof_stash_write(int dirfd, const char *name, const void *buf, size_t size)
> >
> > For consistency this perhaps would be better named dof_stash_write_raw() ?
> 
> Ack, changed. (Originally it wrote out both...)
> 
> > Is there a point to passing in dirfd given that the only caller simply passes
> > in the dof_dir global variable?
> 
> Only that it's passing in a filename too, and I do think it's neater to
> pass in a path and name in that case (even if the path is a dirfd, much
> like the *at() functions), rather than passing in a name and picking up
> the path from somewhere else.
> 
> This is marginal though :)
> 
> >> > +{
> >> > +	struct stat s;
> >> > +	int fd;
> >> > +
> >> > +        /*
> >> > +	 * Sanity check: if the DOF already exists but is not the same size
> >> > +	 * as the DOF we already have, complain, and replace it and its parsed
> >> > +	 * equivalent.  If it does exist, there's no need to write it out.
> >
> > This comment seems to imply that this function replaces the raw DOF and its
> > parsed equivalent.  But the code in this function does not actually do that.
> > So, I assume the comment needs to be updated to reflect reality?
> 
> Ugh, the comment was copied from its caller back in the day when the
> caller also wrote the parsed DOF out (when I thought parsed DOF was
> per-mapping, not per-mapping/per-PID) and not adequately adjusted.
> Fixed.
> 
> >> > +	 *
> >> > +	 * If we can't even unlink it or write it out, we give up -- the stash
> >> > +	 * has failed and we won't be able to do anything it implies.
> >> > +	 *
> >> > +	 * (This is only a crude check -- obviously distinct raw DOF could be
> >> > +	 * the same size by pure chance.)
> >
> > I fear that the check is so crude that it may not catch that many cases.  E.g.
> > when I recompile an executable or library but do not change the probes, it is
> > very likely that the offset location of the probes will change, but the DOF
> > size will not because it is based on statically sized data items.  And so there
> > is a higher chance that two distinct DOF versions will have the same size.
> >
> > Something like a checksum may be needed here.
> 
> As the caller notes, this is actually very hard to hit, hard enough that
> I have no idea how to test it without a custom pathological filesystem:
> you need to kill -9 a process with a mapping with DOF in it, unlink the
> mapping's ELF file, then create a new one *with the same inode number*
> for which the DOF actually has the same size.

I am sorry - I overlooked that the use of $dev-$ino in the name already
introduces a rather good check that two DOFs are different, and yes, the
size check (probably not even necessary in the very vast majority of cases)
adds one extra check.

Good - false alarm.

> Does that seem remotely probable? With inode numbers now chosen from a
> 2^32-wide space and rising to 2^64 on most filesystems (and on most new
> ones, like btrfs and xfs, sparsely allocated within that range) this
> feels like something it is just not worth worrying about, and certainly
> not spending time in a single-threaded process doing checksums.
> 
> I suppose the one alternative is that you could *edit the ELF file* to
> modify the DOF, which surely nothing should ever do, since it's a recipe
> for crashes of shared libraries and -ETXTBSY of main programs? if
> something does it -- say, an attacker -- the consequence of this is that
> the new probably-malicious DOF might be ignored for a while.
> 
> I was more concerned that you'd tell me that this was pointless because
> it's so hard to hit, but I thought I should do *something*, and this at
> least is simple enough that I don't need to worry too much about it
> being nearly impossible to test.
> 
> >> > +	return size + sizeof(uint64_t);
> >> > +
> >> > +  err:
> >
> > Goto labels are typically at the left marging in our code.
> 
> cc-mode strikes again!
> 
> ... presumably this only applies to goto labels at the top level in a
> function. Ones at a lower level (and dof_stash does have a few) would
> look *really ugly* like that.
> 
> Hm, can I teach cc-mode how to do this...
> 
> ... yep! for reference for the zero other of us who are Emacs users:
> 
> (defun c-lineup-top-level-labels-at-0 (_langelem)
>   "Line up labels at the top level at column 0."
>   (save-excursion
>     (back-to-indentation)
>     (if (< (current-column) c-basic-offset)
> 	'-
>       0)))
> 
> (c-add-style "dtrace" '("linux" (c-offsets-alist . ((label . c-lineup-top-level-labels-at-0)))))