Date: Sat, 27 Mar 2021 18:16:44 +0000 From: Jessica Clarke <jrtc27@freebsd.org> To: Mark Johnston <markj@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: <BA0CCFBD-2F63-4561-B6EC-D288D48B8E6A@freebsd.org> In-Reply-To: <202103271806.12RI67Bp061468@gitrepo.freebsd.org> References: <202103271806.12RI67Bp061468@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 27 Mar 2021, at 18:06, Mark Johnston <markj@FreeBSD.org> wrote: >=20 > The branch main has been updated by markj: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3D410556f1f10fd35b350102725fd8504c= 3cb0afc8 >=20 > 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 >=20 > libctf: Fix an out-of-bounds read in ctf_lookup_by_name() >=20 > 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) =3D=3D 1, but then we increment p with the length of = "struct", > resulting in an out of bounds read. >=20 > 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(-) >=20 > 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 */ >=20 > for (lp =3D fp->ctf_lookups; lp->ctl_prefix !=3D NULL; = lp++) { > - if (lp->ctl_prefix[0] =3D=3D '\0' || > - strncmp(p, lp->ctl_prefix, (size_t)(q - p)) = =3D=3D 0) { > + if ((size_t)(q - p) >=3D lp->ctl_len && > + (lp->ctl_prefix[0] =3D=3D '\0' || > + strncmp(p, lp->ctl_prefix, (size_t)(q - p)) = =3D=3D 0)) { > for (p +=3D 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 =E2=80=9Cupstream=E2=80=9D 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=E2=80=99s equivalent, just differently formatted, so adds noise to = the diff. Jess [1] = https://github.com/illumos/illumos-gate/commit/d15d17d4231f87f1571fa6d5853= 77206f360f667
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BA0CCFBD-2F63-4561-B6EC-D288D48B8E6A>