linux-kernel - Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code

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]

Date:   Tue, 4 Jul 2017 23:54:01 +0200
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     patches@...ups.riscv.org
Cc:     peterz@...radead.org, mingo@...hat.com, mcgrof@...nel.org,
        viro@...iv.linux.org.uk, sfr@...b.auug.org.au,
        nicolas.dichtel@...nd.com, rmk+kernel@...linux.org.uk,
        msalter@...hat.com, tklauser@...tanz.ch, will.deacon@....com,
        james.hogan@...tec.com, paul.gortmaker@...driver.com,
        linux@...ck-us.net, linux-kernel@...r.kernel.org,
        linux-arch@...r.kernel.org, albert@...ive.com,
        Palmer Dabbelt <palmer@...belt.com>
Subject: Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code

Hi, below are some small comments.

On Tue, Jul 04, 2017 at 12:50:54PM -0700, Palmer Dabbelt wrote:
> This contains the various __init C functions, the initial assembly
> kernel entry point, and the code to reset the system.  When a file was
> init-related, it contains

It contains what?

> Signed-off-by: Palmer Dabbelt <palmer@...belt.com>
[...]

> +#ifdef CONFIG_GENERIC_BUG
> +#define __BUG_INSN	_AC(0x00100073, UL) /* sbreak */

This should be ebreak, not sbreak, in Priv Spec 1.10, AFAICT.
I guess binutils still understands sbreak, but it's nicer to stick to
the spec, IMHO.

> +#define BUG()							\
> +do {								\
> +	__asm__ __volatile__ (					\
> +		"1:\n\t"					\
> +			"sbreak\n"				\

ebreak

> +			".pushsection __bug_table,\"a\"\n\t"	\
> +		"2:\n\t"					\
> +			__BUG_ENTRY "\n\t"			\
> +			".org 2b + %2\n\t"			\
> +			".popsection"				\
> +		:						\
> +		: "i" (__FILE__), "i" (__LINE__),		\
> +		  "i" (sizeof(struct bug_entry)));		\
> +	unreachable();						\
> +} while (0)
> +#endif /* !__ASSEMBLY__ */
> +#else /* CONFIG_GENERIC_BUG */
> +#ifndef __ASSEMBLY__
> +#define BUG()							\
> +do {								\
> +	__asm__ __volatile__ ("sbreak\n");			\

ebreak

> +	unreachable();						\
> +} while (0)
> +#endif /* !__ASSEMBLY__ */
> +#endif /* CONFIG_GENERIC_BUG */
[...]

> +#define DO_ERROR_INFO(name, signo, code, str)				\
> +asmlinkage void name(struct pt_regs *regs)				\
> +{									\
> +	do_trap_error(regs, signo, code, regs->sepc, "Oops - " str);	\
> +}
> +
> +DO_ERROR_INFO(do_trap_unknown,
> +	SIGILL, ILL_ILLTRP, "unknown exception");
> +DO_ERROR_INFO(do_trap_insn_misaligned,
> +	SIGBUS, BUS_ADRALN, "instruction address misaligned");
> +DO_ERROR_INFO(do_trap_insn_fault,
> +	SIGBUS, BUS_ADRALN, "instruction access fault");

For a general instruction access fault, BUS_ADRALN seems wrong. A
variant of SIGSEGV seems more appropriate, IMHO.


Jonathan Neuschäfer

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)