CTF: Fix an out-of-bounds read
ClosedPublic
Actions

Authored by domagoj.stolfa_gmail.com on Mar 26 2021, 7:18 PM.

Details

Reviewers

markj
gnn

Group Reviewers

DTrace

Commits

rGcbadf77834e1: libctf: Fix an out-of-bounds read in ctf_lookup_by_name()
rG410556f1f10f: libctf: Fix an out-of-bounds read in ctf_lookup_by_name()

Summary

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. This diff simply adds some robustness checks to the surrounding code to prevent this from happening and causing weird behaviour.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

domagoj.stolfa_gmail.com requested review of this revision.Mar 26 2021, 7:18 PM

domagoj.stolfa_gmail.com created this revision.

Thanks, this looks right to me.

This revision is now accepted and ready to land.Mar 27 2021, 2:50 PM

(Please let me know if you'd like me to commit this.)

In D29435#659665, @markj wrote:

(Please let me know if you'd like me to commit this.)

I think this is fine to commit -- I've been running it and ran into no issues. Thanks!

markj added inline comments.Mar 27 2021, 5:52 PM

cddl/contrib/opensolaris/common/ctf/ctf_lookup.c
94	Sorry, I had been focusing on the other change. Could you explain why this part is needed? I'm having trouble seeing how `q` or `p` can end up being NULL.

domagoj.stolfa_gmail.com updated this revision to Diff 86394.Mar 27 2021, 6:01 PM

This revision now requires review to proceed.Mar 27 2021, 6:01 PM

markj accepted this revision as: markj.Mar 27 2021, 6:02 PM

This revision is now accepted and ready to land.Mar 27 2021, 6:02 PM

domagoj.stolfa_gmail.com added inline comments.Mar 27 2021, 6:02 PM

cddl/contrib/opensolaris/common/ctf/ctf_lookup.c
94	Sorry, I had been focusing on the other change. Could you explain why this part is needed? I'm having trouble seeing how `q` or `p` can end up being NULL.
94	You're right. I've updated the diff to reflect it. I think it was left over from debugging when I thought my code had caused a memory corruption :-).