Re: [PATCH v2] riscv: elf: add .riscv.attributes parsing

[Vineet Gupta <vineetg@rivosinc.com>]

On 1/12/23 14:38, Jessica Clarke wrote:
> On 12 Jan 2023, at 21:06, Vineet Gupta<vineetg@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