Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c2177e4e-e97d-448a-73e7-c50622fdc4bb@rivosinc.com>
Date:   Wed, 18 Jan 2023 15:19:15 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
From:   Vineet Gupta <vineetg@rivosinc.com>
Subject: Re: [PATCH v2] riscv: elf: add .riscv.attributes parsing
To:     Jessica Clarke <jrtc27@jrtc27.com>
Cc:     linux-riscv <linux-riscv@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Palmer Dabbelt <palmer@rivosinc.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Eric Biederman <ebiederm@xmission.com>,
        Kees Cook <keescook@chromium.org>, Guo Ren <guoren@kernel.org>,
        Greentime Hu <greentime.hu@sifive.com>,
        Conor Dooley <conor.dooley@microchip.com>, linux@rivosinc.com
References: <20230112210622.2337254-1-vineetg@rivosinc.com>
 <D6A37757-DB22-44C6-A906-D68A0B8BD7A6@jrtc27.com>
Content-Language: en-US
In-Reply-To: <D6A37757-DB22-44C6-A906-D68A0B8BD7A6@jrtc27.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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