Messages in this thread | | | Date | Thu, 19 Jan 2023 14:05:47 -0800 | Subject | Re: [PATCH v3] riscv: elf: add .riscv.attributes parsing | From | Vineet Gupta <> |
| |
On 1/19/23 12:33, Jessica Clarke wrote: >> + >> +/* >> + * Parse a single elf attribute. >> + */ >> +static int rv_parse_elf_attr_safe(unsigned char **dpp, unsigned char *p_end) >> +{ >> + unsigned char *p = *dpp; >> + unsigned char *str; >> + u64 tag, val; >> + u32 s_len; >> + >> + if (decode_uleb128_safe(&p, &tag, p_end)) >> + goto bad_attr; >> + >> + switch (tag) { >> + case RV_ATTR_TAG_stack_align: >> + if (decode_uleb128_safe(&p, &val, p_end)) >> + goto bad_attr; >> + if (!ELF_ATTR_TAG_EVEN(tag)) >> + goto bad_attr; > Huh? You just checked it against a constant so you know exactly what it > is. This is just Static_assert(RV_ATTR_TAG_stack_align % 2 == 0) but at > run time. And you know that’s going to be the case, because the spec is > self-consistent by design (wouldn’t be a worthwhile spec otherwise).
Makes sense.
>> + rv_elf_attr_int(tag, val); >> + break; >> + >> + case RV_ATTR_TAG_unaligned_access: >> + if (decode_uleb128_safe(&p, &val, p_end)) >> + goto bad_attr; >> + if (!ELF_ATTR_TAG_EVEN(tag)) >> + goto bad_attr; >> + rv_elf_attr_int(tag, val); >> + break; >> + >> + case RV_ATTR_TAG_arch: >> + if (ELF_ATTR_TAG_EVEN(tag)) >> + goto bad_attr; >> + str = p; >> + s_len = strnlen(p, p_end - p) + 1; >> + if ((p + s_len) > p_end) >> + goto bad_attr; >> + p += s_len; >> + rv_elf_attr_str(tag, str); >> + break; >> + >> + default: > The whole point of the even/odd split is so that when you *don’t* know > what the tag means you can still decode its value and thus know how to > skip past it. That is, *here* is where you need to be checking > even/odd, and deciding whether to treat it as a string or a ULEB128,
I see the point. We can ignore the specific tags and just treat odd and even tags as string and int respectively. And keep a loose check of the known tags vs. unknown.
> which is why I annotated *here* not one of the other case labels before.
OK. I really need to pay attention to what and where to your comments :-)
> >> + memset(buf, 0, RV_ATTR_SEC_SZ); >> + pos = phdr->p_offset; >> + n = kernel_read(f, &buf, phdr->p_filesz, &pos); >> + >> + if (n <= 0) >> + return -EIO; > 0 should be ENOEXEC not EIO? And surely in the < 0 case you want to be > forwarding on the exact error from kernel_read, not squashing it into > EIO?
Right.
> >> +/* >> + * Hook invoked by generic elf loader to parse riscv specific elf segments. >> + */ >> +int arch_elf_pt_proc(void *_ehdr, void *_phdr, struct file *elf, >> + bool is_interp, struct arch_elf_state *state) >> +{ >> + struct elf_phdr *phdr = _phdr; >> + int ret = 0; >> + >> + if (phdr->p_type == PT_RISCV_ATTRIBUTES && !is_interp) > Both the executable and its interpreter matter.
OK.
Thx, -Vineet
| |