linux-kernel - Re: [PATCH] MIPS: R6: emulation of PC-relative instructions

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] [day] [month] [year] [list]

Date:	Tue, 11 Aug 2015 11:24:07 -0700
From:	Leonid Yegoshin <Leonid.Yegoshin@...tec.com>
To:	Markos Chandras <markos.chandras@...tec.com>
CC:	<linux-mips@...ux-mips.org>, <paul.burton@...tec.com>,
	<david.daney@...ium.com>, <Steven.Hill@...tec.com>,
	<linux-kernel@...r.kernel.org>, <ralf@...ux-mips.org>,
	<macro@...ux-mips.org>
Subject: Re: [PATCH] MIPS: R6: emulation of PC-relative instructions

On 08/11/2015 07:41 AM, Markos Chandras wrote:
> Hi,
>
> On Wed, Aug 05, 2015 at 04:53:43PM -0700, Leonid Yegoshin wrote:
>>   			if (nir) {
>>   				err = mipsr6_emul(regs, nir);
>>   				if (err > 0) {
>> +					regs->cp0_epc = nepc;
> Does this change belog to this patch? If so why?

Yes, it is needed for correct address calculation. Until PC-relative 
instruction emulation it was not needed in dsemul().

>   Maybe a comment would help?
> It does feel like it fixes a different problem but I haven't read your patch in depth.
>
>>   
>>
>>   
>>   #include "ieee754.h"
>>   
>> +#ifdef CONFIG_CPU_MIPSR6
> Can we simply avoid the if/def for R6 please? Just leave this function as is and
> use if(cpu_has_mips_r6) when calling it. If you can't do that, please explain
> why.
Yes, we can. But we have a bunch of that in many places and somewhere it 
is difficult to avoid "#ifdef".
I just like to have an uniform standard.

Besides that "#ifdef" assures quickly that it is a build time choice but 
not runtime.

>> +
>> +static int mipsr6_pc(struct pt_regs *regs, mips_instruction inst, unsigned long cpc,
>> +		    unsigned long bpc, unsigned long r31)
>> +{
>> +	union mips_instruction ir = (union mips_instruction)inst;
>> +	register unsigned long vaddr;
>> +	unsigned int val;
>> +	int err = SIGILL;
>> +
>> +	if (ir.rel_format.opcode != pcrel_op)
>> +		return SIGILL;
>> +
>> +	switch (ir.rel_format.op) {
>> +	case addiupc_op:
>> +		vaddr = regs->cp0_epc + (ir.rel_format.simmediate << 2);
>> +		if (config_enabled(CONFIG_64BIT) && !(regs->cp0_status & ST0_UX))
>> +			__asm__ __volatile__("sll %0, %0, 0":"+&r"(vaddr)::);
>> +		regs->regs[ir.rel_format.rs] = vaddr;
>> +		return 0;
>> +#ifdef CONFIG_CPU_MIPS64
> Could you use cpu_has_mips64 and avoid the if/def

No we can't - cpu_has_mips64 is a CPU ISA feature but here is a kernel 
code capability restriction. The difference happens during running 
MIPS32 kernel on MIPS64 processor. We should not emulate MIPS64 
instructions on MIPS32 kernel.

>   and return SIGILL if it is not
> true?

The common return standard for emulation functions in MIPS is:

     0 - OK, emulation done
     SIGILL - doesn't recognize an instruction, it still may be some 
another way to fix a problem or SIGILL.
     other - some trouble or whatever (non-standardized, in MIPS R2 
emulator it has subcodes)

I don't see a reason to change it and have here a special standard.

- Leonid.

>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/