linux-kernel - Re: [PATCH 2/9] ARM: traps: use get_kernel_nofault instead of set

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, 8 Sep 2020 08:15:28 +0200
From:   Christoph Hellwig <hch@....de>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Christoph Hellwig <hch@....de>,
        Russell King <rmk@....linux.org.uk>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linus.walleij@...aro.org,
        Russell King <linux@...linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/9] ARM: traps: use get_kernel_nofault instead of
 set_fs()

> +static void dump_mem(const char *, const char *, unsigned long, unsigned long, bool kernel_mode);

This adds a pointlessly long line.  

And looking at the code I don't see why the argument is even needed.

dump_mem() currently does an unconditional set_fs(KERNEL_DS), so it
should always use get_kernel_nofault.

> +static void dump_instr(const char *lvl, struct pt_regs *regs)
>  {
>  	unsigned long addr = instruction_pointer(regs);
>  	const int thumb = thumb_mode(regs);
> @@ -173,10 +169,20 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
>  	for (i = -4; i < 1 + !!thumb; i++) {
>  		unsigned int val, bad;
>  
> -		if (thumb)
> -			bad = get_user(val, &((u16 *)addr)[i]);
> -		else
> -			bad = get_user(val, &((u32 *)addr)[i]);
> +		if (!user_mode(regs)) {
> +			if (thumb) {
> +				u16 val16;
> +				bad = get_kernel_nofault(val16, &((u16 *)addr)[i]);
> +				val = val16;
> +			} else {
> +				bad = get_kernel_nofault(val, &((u32 *)addr)[i]);
> +			}
> +		} else {
> +			if (thumb)
> +				bad = get_user(val, &((u16 *)addr)[i]);
> +			else
> +				bad = get_user(val, &((u32 *)addr)[i]);
> +		}

When I looked at this earlier I just added a little helper to make
this a little easier to read.   Here is my patch from an old tree:

http://git.infradead.org/users/hch/misc.git/commitdiff/67413030ccb7a64a7eb828e13ff0795f4eadfeb7