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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c2177e4e-e97d-448a-73e7-c50622fdc4bb@rivosinc.com>
Date:   Wed, 18 Jan 2023 15:19:15 -0800
From:   Vineet Gupta <vineetg@...osinc.com>
To:     Jessica Clarke <jrtc27@...c27.com>
Cc:     linux-riscv <linux-riscv@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Palmer Dabbelt <palmer@...osinc.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Eric Biederman <ebiederm@...ssion.com>,
        Kees Cook <keescook@...omium.org>, Guo Ren <guoren@...nel.org>,
        Greentime Hu <greentime.hu@...ive.com>,
        Conor Dooley <conor.dooley@...rochip.com>, linux@...osinc.com
Subject: Re: [PATCH v2] riscv: elf: add .riscv.attributes parsing



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