netdev - RE: [5.3] ucc_geth: Fix to avoid IS_ERR_VALUE abuses and dead code on 64bit systems.

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]

Message-ID: <063D6719AE5E284EB5DD2968C1650D6D5F50C493@AcuExch.aculab.com>
Date:	Mon, 8 Aug 2016 14:49:11 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Arvind Yadav' <arvind.yadav.cs@...il.com>,
	"zajec5@...il.com" <zajec5@...il.com>,
	"leoli@...escale.com" <leoli@...escale.com>
CC:	"qiang.zhao@...escale.com" <qiang.zhao@...escale.com>,
	"scottwood@...escale.com" <scottwood@...escale.com>,
	"viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux@...ck-us.net" <linux@...ck-us.net>,
	"arnd@...db.de" <arnd@...db.de>
Subject: RE: [5.3] ucc_geth: Fix to avoid IS_ERR_VALUE abuses and dead code
 on 64bit systems.

From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org] On Behalf Of Arvind Yadav
> IS_ERR_VALUE() assumes that parameter is an unsigned long.
> It can not be used to check if 'unsigned int' is passed insted.
> Which tends to reflect an error.
> In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
> IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).
> IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
> value is zero extended to 64 bits.

You are being far too wordy above, and definitely below.

> 
> Now problem in Freescale QEGigabit Ethernet-:
>                                  drivers/net/ethernet/freescale/ucc_geth.c
> 
...
>          qe_muram_addr(init_enet_pram_offset);
> 
> qe_muram_alloc (a.k.a. cpm_muram_alloc) returns unsigned long.
> Return value store in a u32 (init_enet_offset, exf_glbl_param_offset,
> rx_glbl_pram_offset, tx_glbl_pram_offset, send_q_mem_reg_offset,
> thread_dat_tx_offset, thread_dat_rx_offset, scheduler_offset,
> tx_fw_statistics_pram_offset, rx_fw_statistics_pram_offset,
> rx_irq_coalescing_tbl_offset, rx_bd_qs_tbl_offset, tx_bd_ring_offset,
> init_enet_pram_offset and rx_bd_ring_offset).

Inpenetrable...

> If qe_muram_alloc will return any error, Then IS_ERR_VALUE will always
> return 0. it'll not call ucc_fast_free for any failure. Inside 'if code'
> will be a dead code on 64bit. Even qe_muram_addr will return wrong
> virtual address. Which can cause an error.
> 
>  kfree((void *)ugeth->tx_bd_ring_offset[i]);

Erm, kfree() isn't the right function for things allocated by qe_muram_alloc().

I still thing you need to stop this code using IS_ERR_VALUE() at all.

	David