[DTrace-devel] [PATCH 3/4] uregs: Fix access to thread members on x86

[Nick Alcock]

On 18 May 2023, eugene loh uttered the following:

> From: Eugene Loh <eugene.loh at oracle.com>
>
> The "fix" is simply to account for thread members that are less than
> 8 bytes wide.
>
> Most of this patch is for refactoring (using functions to determine
> offsets with CTF types or to load a scalar safely from memory that
> the BPF verifier cannot vet) and adding a test.

Hmm...

>  		if (idx <= 25) {

You check if idx is <= 25 here, and if it's too large just above...

> -			ctf_file_t *cfp = dtp->dt_shared_ctf;
> -			ctf_id_t type;
> -			ctf_membinfo_t ctm;
> -			int offset, rc;
> -
> -			/* look up task->thread offset */
> -			if (!cfp)
> -				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> -
> -			type = ctf_lookup_by_name(cfp, "struct task_struct");
> -			if (type == CTF_ERR)
> -				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> -			if (ctf_member_info(cfp, type, "thread", &ctm) == CTF_ERR)
> -				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> -			offset = ctm.ctm_offset / NBBY;
> -
> -			/* add the thread->member offset */
> -			type = ctf_lookup_by_name(cfp, "struct thread_struct");
> -			if (type == CTF_ERR)
> -				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> -			switch (idx) {
> -			case 21: rc = ctf_member_info(cfp, type, "ds", &ctm); break;
> -			case 22: rc = ctf_member_info(cfp, type, "es", &ctm); break;
> -			case 23: rc = ctf_member_info(cfp, type, "fsbase", &ctm); break;
> -			case 24: rc = ctf_member_info(cfp, type, "gsbase", &ctm); break;
> -			case 25: rc = ctf_member_info(cfp, type, "trap_nr", &ctm); break;
> -			}
> -			if (rc == -1)
> -				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);

... and this removed code handled the case where it was < 21 and
dt_pt_regs is smaller than 20...

> -			offset += ctm.ctm_offset / NBBY;
> +			int offset;
> +			ssize_t size;
> +			char *memnames[] = { "ds", "es", "fsbase", "gsbase", "trap_nr" };
> +
> +			/* Look up task->thread offset. */
> +			offset = dt_cg_ctf_offsetof("struct task_struct", "thread", NULL);
> +
> +			/* Add the thread->member offset. */
> +			offset += dt_cg_ctf_offsetof("struct thread_struct",
> +						     memnames[idx - 21], &size);

... but this just walks blindly off the start of the array if idx < 21
and dt_pt_regs is smaller than you think it is. I don't see anything
checking for this in dt_cg_uregs()'s only caller. I mean yes pt_regs is
in the ABI but this sort of implicit reliance on type sizes or you walk
off the start of an array makes me come out in hives.

Maybe an assert() here might be a good idea? I'd rather have an
assertion failure than a buffer underrun.

-- 
NULL && (void)