netdev - Re: Memory corruption with r8169 across several device revisions and kernels

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:   Wed, 24 Jan 2018 02:21:55 +0100
From:   Oliver Freyermuth <o.freyermuth@...glemail.com>
To:     Francois Romieu <romieu@...zoreil.com>
Cc:     netdev@...r.kernel.org
Subject: Re: Memory corruption with r8169 across several device revisions and
 kernels

Am 23.01.2018 um 23:13 schrieb Francois Romieu:
> 
> It helps. Can you try the snippet below ?

It seems to fix the issue - I could not reproduce memory corruption anymore
neither on an Ubuntu 17.10.1 live system (with patched kernel module)
nor on my Gentoo system (4.14.12 with your patch applied) 
across several reboots and module reloads! 

Sorry for my ignorance, but I'm curious - the R32 is there to avoid reordering of the writes? 

Also, a small issue (for the final patch, in case you did not notice): 
"bool ret" is now unused and will produce a compiler warning. 

Many thanks for the quick help. I don't know the policies, but from user point of view,
this should be a good candidate for backporting to stable kernels, 
since many systems in the wild should be affected by this,
and spurious memory corruption leading to e.g. broken filesystems is rather nasty. 

All the best,
	Oliver

> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 272c5962e4f7..8531b41e3397 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -2238,16 +2238,12 @@ static bool rtl8169_do_counters(struct net_device *dev, u32 counter_cmd)
>  	bool ret;
>  
>  	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
> +	RTL_R32(CounterAddrHigh);
>  	cmd = (u64)paddr & DMA_BIT_MASK(32);
>  	RTL_W32(CounterAddrLow, cmd);
>  	RTL_W32(CounterAddrLow, cmd | counter_cmd);
>  
> -	ret = rtl_udelay_loop_wait_low(tp, &rtl_counters_cond, 10, 1000);
> -
> -	RTL_W32(CounterAddrLow, 0);
> -	RTL_W32(CounterAddrHigh, 0);
> -
> -	return ret;
> +	return rtl_udelay_loop_wait_low(tp, &rtl_counters_cond, 10, 1000);
>  }
>  
>  static bool rtl8169_reset_counters(struct net_device *dev)
>