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

[Vineet Gupta]

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