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: <67c50e8d-ae78-076d-cf25-b7781f2209d7@rivosinc.com>
Date:   Thu, 19 Jan 2023 14:05:47 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v3] riscv: elf: add .riscv.attributes parsing
Content-Language: en-US
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>,
        Andy Chiu <andy.chiu@sifive.com>,
        Conor Dooley <conor.dooley@microchip.com>, linux@rivosinc.com
References: <c2177e4e-e97d-448a-73e7-c50622fdc4bb@rivosinc.com>
 <20230119174357.3550008-1-vineetg@rivosinc.com>
 <EB6B06BB-C65F-41C2-8D9F-F916F3A706FA@jrtc27.com>
From:   Vineet Gupta <vineetg@rivosinc.com>
In-Reply-To: <EB6B06BB-C65F-41C2-8D9F-F916F3A706FA@jrtc27.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 1/19/23 12:33, Jessica Clarke wrote:
>> +
>> +/*
>> + * Parse a single elf attribute.
>> + */
>> +static int rv_parse_elf_attr_safe(unsigned char **dpp, unsigned char *p_end)
>> +{
>> +	unsigned char *p = *dpp;
>> +	unsigned char *str;
>> +	u64 tag, val;
>> +	u32 s_len;
>> +
>> +	if (decode_uleb128_safe(&p, &tag, p_end))
>> +		goto bad_attr;
>> +
>> +	switch (tag) {
>> +	case RV_ATTR_TAG_stack_align:
>> +		if (decode_uleb128_safe(&p, &val, p_end))
>> +			goto bad_attr;
>> +		if (!ELF_ATTR_TAG_EVEN(tag))
>> +			goto bad_attr;
> Huh? You just checked it against a constant so you know exactly what it
> is. This is just Static_assert(RV_ATTR_TAG_stack_align % 2 == 0) but at
> run time. And you know that’s going to be the case, because the spec is
> self-consistent by design (wouldn’t be a worthwhile spec otherwise).

Makes sense.

>> +		rv_elf_attr_int(tag, val);
>> +		break;
>> +
>> +	case RV_ATTR_TAG_unaligned_access:
>> +		if (decode_uleb128_safe(&p, &val, p_end))
>> +			goto bad_attr;
>> +		if (!ELF_ATTR_TAG_EVEN(tag))
>> +			goto bad_attr;
>> +		rv_elf_attr_int(tag, val);
>> +		break;
>> +
>> +	case RV_ATTR_TAG_arch:
>> +		if (ELF_ATTR_TAG_EVEN(tag))
>> +			goto bad_attr;
>> +		str = p;
>> +		s_len = strnlen(p, p_end - p) + 1;
>> +		if ((p + s_len) > p_end)
>> +			goto bad_attr;
>> +		p += s_len;
>> +		rv_elf_attr_str(tag, str);
>> +		break;
>> +
>> +	default:
> The whole point of the even/odd split is so that when you *don’t* know
> what the tag means you can still decode its value and thus know how to
> skip past it. That is, *here* is where you need to be checking
> even/odd, and deciding whether to treat it as a string or a ULEB128,

I see the point. We can ignore the specific tags and just treat odd and 
even tags as string and int respectively.
And keep a loose check of the known tags vs. unknown.

> which is why I annotated *here* not one of the other case labels before.

OK. I really need to pay attention to what and where to your comments :-)


>
>> +	memset(buf, 0, RV_ATTR_SEC_SZ);
>> +	pos = phdr->p_offset;
>> +	n = kernel_read(f, &buf, phdr->p_filesz, &pos);
>> +
>> +	if (n <= 0)
>> +		return -EIO;
> 0 should be ENOEXEC not EIO? And surely in the < 0 case you want to be
> forwarding on the exact error from kernel_read, not squashing it into
> EIO?

Right.

>
>> +/*
>> + * Hook invoked by generic elf loader to parse riscv specific elf segments.
>> + */
>> +int arch_elf_pt_proc(void *_ehdr, void *_phdr, struct file *elf,
>> +		     bool is_interp, struct arch_elf_state *state)
>> +{
>> +	struct elf_phdr *phdr = _phdr;
>> +	int ret = 0;
>> +
>> +	if (phdr->p_type == PT_RISCV_ATTRIBUTES && !is_interp)
> Both the executable and its interpreter matter.

OK.

Thx,
-Vineet