linux-kernel - Re: [PATCH] net: fec: limit register access on i.MX6UL

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: <20220920115624.zgycbbzijudr7muc@pengutronix.de>
Date:   Tue, 20 Sep 2022 13:56:24 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Juergen Borleis <jbe@...gutronix.de>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH] net: fec: limit register access on i.MX6UL

On 20.09.2022 11:51:06, Juergen Borleis wrote:
> Using 'ethtool -d […]' on an i.MX6UL leads to a kernel crash:
> 
>    Unhandled fault: external abort on non-linefetch (0x1008) at […]
> 
> due to this SoC has less registers in its FEC implementation compared to other
> i.MX6 variants. Thus, a run-time decision is required to avoid access to
> non-existing registers.
> 
> Signed-off-by: Juergen Borleis <jbe@...gutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++++++++++++--
>  1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 6152f6d..ab620b4 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2382,6 +2382,31 @@ static u32 fec_enet_register_offset[] = {
>  	IEEE_R_DROP, IEEE_R_FRAME_OK, IEEE_R_CRC, IEEE_R_ALIGN, IEEE_R_MACERR,
>  	IEEE_R_FDXFC, IEEE_R_OCTETS_OK
>  };
> +/* for i.MX6ul */
> +static u32 fec_enet_register_offset_6ul[] = {
> +	FEC_IEVENT, FEC_IMASK, FEC_R_DES_ACTIVE_0, FEC_X_DES_ACTIVE_0,
> +	FEC_ECNTRL, FEC_MII_DATA, FEC_MII_SPEED, FEC_MIB_CTRLSTAT, FEC_R_CNTRL,
> +	FEC_X_CNTRL, FEC_ADDR_LOW, FEC_ADDR_HIGH, FEC_OPD, FEC_TXIC0, FEC_RXIC0,
> +	FEC_HASH_TABLE_HIGH, FEC_HASH_TABLE_LOW, FEC_GRP_HASH_TABLE_HIGH,
> +	FEC_GRP_HASH_TABLE_LOW, FEC_X_WMRK, FEC_R_DES_START_0,
> +	FEC_X_DES_START_0, FEC_R_BUFF_SIZE_0, FEC_R_FIFO_RSFL, FEC_R_FIFO_RSEM,
> +	FEC_R_FIFO_RAEM, FEC_R_FIFO_RAFL, FEC_RACC,
> +	RMON_T_DROP, RMON_T_PACKETS, RMON_T_BC_PKT, RMON_T_MC_PKT,
> +	RMON_T_CRC_ALIGN, RMON_T_UNDERSIZE, RMON_T_OVERSIZE, RMON_T_FRAG,
> +	RMON_T_JAB, RMON_T_COL, RMON_T_P64, RMON_T_P65TO127, RMON_T_P128TO255,
> +	RMON_T_P256TO511, RMON_T_P512TO1023, RMON_T_P1024TO2047,
> +	RMON_T_P_GTE2048, RMON_T_OCTETS,
> +	IEEE_T_DROP, IEEE_T_FRAME_OK, IEEE_T_1COL, IEEE_T_MCOL, IEEE_T_DEF,
> +	IEEE_T_LCOL, IEEE_T_EXCOL, IEEE_T_MACERR, IEEE_T_CSERR, IEEE_T_SQE,
> +	IEEE_T_FDXFC, IEEE_T_OCTETS_OK,
> +	RMON_R_PACKETS, RMON_R_BC_PKT, RMON_R_MC_PKT, RMON_R_CRC_ALIGN,
> +	RMON_R_UNDERSIZE, RMON_R_OVERSIZE, RMON_R_FRAG, RMON_R_JAB,
> +	RMON_R_RESVD_O, RMON_R_P64, RMON_R_P65TO127, RMON_R_P128TO255,
> +	RMON_R_P256TO511, RMON_R_P512TO1023, RMON_R_P1024TO2047,
> +	RMON_R_P_GTE2048, RMON_R_OCTETS,
> +	IEEE_R_DROP, IEEE_R_FRAME_OK, IEEE_R_CRC, IEEE_R_ALIGN, IEEE_R_MACERR,
> +	IEEE_R_FDXFC, IEEE_R_OCTETS_OK
> +};
>  #else
>  static __u32 fec_enet_register_version = 1;
>  static u32 fec_enet_register_offset[] = {
> @@ -2406,7 +2431,26 @@ static void fec_enet_get_regs(struct net_device *ndev,
>  	u32 *buf = (u32 *)regbuf;
>  	u32 i, off;
>  	int ret;
> -
> +#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
> +	defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM) || \
> +	defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
> +	struct platform_device_id *dev_info =
> +			(struct platform_device_id *)fep->pdev->id_entry;
> +	u32 *reg_list;
> +	u32 reg_cnt;
> +
> +	if (strcmp(dev_info->name, "imx6ul-fec")) {
> +		reg_list = fec_enet_register_offset;
> +		reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> +	} else {
> +		reg_list = fec_enet_register_offset_6ul;
> +		reg_cnt = ARRAY_SIZE(fec_enet_register_offset_6ul);
> +	}

What about using of_machine_is_compatible()?

> +#else
> +	/* coldfire */
> +	static u32 *reg_list = fec_enet_register_offset;
> +	static const u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> +#endif

Why do you need the ifdef?

>  	ret = pm_runtime_resume_and_get(dev);
>  	if (ret < 0)
>  		return;
> @@ -2415,8 +2459,8 @@ static void fec_enet_get_regs(struct net_device *ndev,
>  
>  	memset(buf, 0, regs->len);
>  
> -	for (i = 0; i < ARRAY_SIZE(fec_enet_register_offset); i++) {
> -		off = fec_enet_register_offset[i];
> +	for (i = 0; i < reg_cnt; i++) {
> +		off = reg_list[i];
>  
>  		if ((off == FEC_R_BOUND || off == FEC_R_FSTART) &&
>  		    !(fep->quirks & FEC_QUIRK_HAS_FRREG))

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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