[PATCH v2] riscv: elf: add .riscv.attributes parsing
Vineet Gupta
vineetg at rivosinc.com
Wed Jan 18 15:19:15 PST 2023
On 1/12/23 14:38, Jessica Clarke wrote:
> On 12 Jan 2023, at 21:06, Vineet Gupta<vineetg at rivosinc.com> wrote:
>> +static int
>> +decode_uleb128_safe(unsigned char **dpp, u32 *val, const unsigned char *p_end)
>> +{
>> + unsigned char *bp = *dpp;
>> + unsigned char byte;
>> + unsigned int shift = 0;
>> + u32 result = 0;
Ved commented off-list about u32 being wide enough. So I'll be making
@val u64 everywhere.
>> + int ok = 0;
>> +
>> + while (bp < p_end) {
>> + byte = *bp++;
>> + result |= (byte & 0x7f) << shift;
>> + if ((byte & 0x80) == 0) {
>> + ok = 1;
>> + break;
> Why not just do the return here?
I guess I could.
>> +
>> + case RV_ATTR_TAG_arch:
>> + str = p;
>> + s_len = strnlen(p, p_end - p) + 1;
>> + p += s_len;
>> + if (p > p_end)
> Constructing such a p is UB, check s_len before instead.
OK.
>> + goto bad_attr;
>> + rv_elf_attr_str(tag, str);
>> + break;
>> +
>> + default:
>> + if (decode_uleb128_safe(&p, &val, p_end))
> From the ratified spec:
>
> "RISC-V attributes have a string value if the tag number is odd and an integer value if the tag number is even."
OK, added sanity checks.
>
>> +
>> + memset(buf, 0, RV_ATTR_SEC_SZ);
> This will hide bugs from sanitisers...
And if kernel_read() fills it partially, leave the rest uninitialized ?
I'll keep the memset.
>> + pos = phdr->p_offset;
>> + n = kernel_read(f, &buf, phdr->p_filesz, &pos);
>> +
>> + if (n < 0)
>> + return -EIO;
>> +
>> + p = buf;
>> + p_end = p + n;
>> +
>> + /* sanity check format-version */
>> + if (*p++ != 'A')
> What if n is 0?
I can check for n <= 0 above.
>> + goto bad_elf;
>> +
>> + /*
>> + * elf attribute section organized as Vendor sub-sections(s)
>> + * {sub-section length, vendor name, vendor data}
>> + * Vendor data organized as sub-subsection(s)
>> + * {tag, sub-subsection length, attributes contents}
>> + * Attribute contents organized as
>> + * {tag, value} pair(s).
>> + */
>> + while ((p_end - p) >= 4) {
>> + int sub_len, vname_len;
> u32?
OK.
>> +
>> + sub_len = get_unaligned_le32(p);
>> + if (sub_len <= 4 || sub_len > n)
> n is the total amount read in, not the remaining amount.
Fixed to sub_len > (p_end - p)
>
>> +
>> + /* Vendor data: sub-subsections(s) */
>> + while (sub_len > 0) {
>> + u32 tag, content_len;
>> + unsigned char *sub_end, *sub_start = p;
> Confusing naming for sub-subsection variables.
Fair enough: p_ss_start, p_ss_end, ss_len
Thx.
-Vineet
More information about the linux-riscv
mailing list