Date: Sat, 27 Mar 2021 14:44:46 -0400 From: Mark Johnston <markj@freebsd.org> To: Jessica Clarke <jrtc27@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: 410556f1f10f - main - libctf: Fix an out-of-bounds read in ctf_lookup_by_name() Message-ID: <YF99HpNZwsa5cSmw@nuc> In-Reply-To: <BA0CCFBD-2F63-4561-B6EC-D288D48B8E6A@freebsd.org> References: <202103271806.12RI67Bp061468@gitrepo.freebsd.org> <BA0CCFBD-2F63-4561-B6EC-D288D48B8E6A@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Mar 27, 2021 at 06:16:44PM +0000, Jessica Clarke wrote: > On 27 Mar 2021, at 18:06, Mark Johnston <markj@FreeBSD.org> wrote: > > > > The branch main has been updated by markj: > > > > URL: https://cgit.FreeBSD.org/src/commit/?id=410556f1f10fd35b350102725fd8504c3cb0afc8 > > > > commit 410556f1f10fd35b350102725fd8504c3cb0afc8 > > Author: Domagoj Stolfa <domagoj.stolfa@gmail.com> > > AuthorDate: 2021-03-27 18:04:12 +0000 > > Commit: Mark Johnston <markj@FreeBSD.org> > > CommitDate: 2021-03-27 18:04:12 +0000 > > > > libctf: Fix an out-of-bounds read in ctf_lookup_by_name() > > > > When prefixes such as struct, union, etc. are compared with the current > > type (e.g. struct foo), a comparison is made with the prefix. The code > > currently assumes that every type is a valid C type with a prefix, > > however at times, garbage ends up in this function causing an > > unpredictable crash with DTrace due to the isspace(*p) call or > > subsequent calls. An example that I've seen of this is the letter 's' > > being passed in, comparing true with struct as the comparison size was > > (q - p) == 1, but then we increment p with the length of "struct", > > resulting in an out of bounds read. > > > > Reviewed by: markj > > MFC after: 1 week > > Differential Revision: https://reviews.freebsd.org/D29435 > > --- > > cddl/contrib/opensolaris/common/ctf/ctf_lookup.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/cddl/contrib/opensolaris/common/ctf/ctf_lookup.c b/q > > index aa58663309b6..5912cc1a36e8 100644 > > --- a/cddl/contrib/opensolaris/common/ctf/ctf_lookup.c > > +++ b/cddl/contrib/opensolaris/common/ctf/ctf_lookup.c > > @@ -132,8 +132,9 @@ ctf_lookup_by_name(ctf_file_t *fp, const char *name) > > continue; /* skip qualifier keyword */ > > > > for (lp = fp->ctf_lookups; lp->ctl_prefix != NULL; lp++) { > > - if (lp->ctl_prefix[0] == '\0' || > > - strncmp(p, lp->ctl_prefix, (size_t)(q - p)) == 0) { > > + if ((size_t)(q - p) >= lp->ctl_len && > > + (lp->ctl_prefix[0] == '\0' || > > + strncmp(p, lp->ctl_prefix, (size_t)(q - p)) == 0)) { > > for (p += lp->ctl_len; isspace(*p); p++) > > continue; /* skip prefix and next ws */ > > We had a student porting DTrace to CheriBSD as a Master's project notice this > and get this fixed “upstream” in Illumos[1], but neglected to do so in FreeBSD > (and it seems CheriBSD has an earlier version of the patch that I requested be > changed in the upstream review...); you might wish to pull that in instead? > It’s equivalent, just differently formatted, so adds noise to the diff. Ah, I had pondered suggesting the exact same change. I think we can just apply this adjustment as a separate diff. I posted the diff while I rerun the test suite: https://reviews.freebsd.org/D29448 > > Jess > > [1] https://github.com/illumos/illumos-gate/commit/d15d17d4231f87f1571fa6d585377206f360f667 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YF99HpNZwsa5cSmw>