netdev - RE: net: fec: memory corruption caused by err_enet_mii

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, 22 Dec 2021 08:01:52 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Kegl Rohit <keglrohit@...il.com>, netdev <netdev@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>
Subject: RE: net: fec: memory corruption caused by err_enet_mii_probe error
 path


Hi Kegl,

Removing Andy Duan, since he has left NXP, and mail is unavailable any longer, to avoid noise.

> -----Original Message-----
> From: Kegl Rohit <keglrohit@...il.com>
> Sent: 2021年12月22日 15:06
> To: netdev <netdev@...r.kernel.org>
> Cc: Andy Duan <fugang.duan@....com>
> Subject: net: fec: memory corruption caused by err_enet_mii_probe error
> path
> 
> Hello!
> 
> There is an issue with the error path of fec_enet_mii_probe in
> fec_enet_open(struct net_device *ndev) which leads to random memory
> corruption.
> 
> In open() the buffers are initialized:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> b.com%2Ftorvalds%2Flinux%2Fblob%2Fv5.10%2Fdrivers%2Fnet%2Fethernet
> %2Ffreescale%2Ffec_main.c%23L3001&amp;data=04%7C01%7Cqiangqing.zh
> ang%40nxp.com%7Cc7511826d9684e2b9d3508d9c51991d0%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C1%7C637757535872985607%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000&amp;sdata=FovOc4z3RIuatEraWWO8lgco8sIea8B0
> 3RVKNd83QZc%3D&amp;reserved=0
> 
> Then fec_restart will start the DMA engines.
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> b.com%2Ftorvalds%2Flinux%2Fblob%2Fv5.10%2Fdrivers%2Fnet%2Fethernet
> %2Ffreescale%2Ffec_main.c%23L3006&amp;data=04%7C01%7Cqiangqing.zh
> ang%40nxp.com%7Cc7511826d9684e2b9d3508d9c51991d0%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C1%7C637757535872985607%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000&amp;sdata=FAEb9MAGVmVYv4gdiJrwhT7k4Pys8
> UlRLnydpAJ6N94%3D&amp;reserved=0
> 
> Now if fec_enet_mii_probe fails (e.g. phy did not respond via mii) the
> err_enet_mii_probe error path will be used
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> b.com%2Ftorvalds%2Flinux%2Fblob%2Fv5.10%2Fdrivers%2Fnet%2Fethernet
> %2Ffreescale%2Ffec_main.c%23L3031&amp;data=04%7C01%7Cqiangqing.zh
> ang%40nxp.com%7Cc7511826d9684e2b9d3508d9c51991d0%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C1%7C637757535872985607%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000&amp;sdata=QLT9%2BcOkH7%2BdRSZzdIzp6oq0rc
> wnOy0vfVN7Sa4YTAM%3D&amp;reserved=0
> 
> err_enet_mii_probe:
> fec_enet_free_buffers(ndev);
> err_enet_alloc:
> fec_enet_clk_enable(ndev, false);
> clk_enable:
> pm_runtime_mark_last_busy(&fep->pdev->dev);
> pm_runtime_put_autosuspend(&fep->pdev->dev);
> pinctrl_pm_select_sleep_state(&fep->pdev->dev);
> return ret;
> 
> This error path frees the DMA buffers, BUT as far I could see it does not stop
> the DMA engines.
> => open() fails => frees buffers => DMA still active => MAC receives network
> packet => DMA starts => random memory corruption (use after
> free) => random kernel panics

A question here, why receive path still active? MAC has not connected to PHY when this failure happened, should not see network activities.

> So maybe fec_stop() as counterpart to fec_restart() is missing before freeing
> the buffers?
> err_enet_mii_probe:
> fec_stop(ndev);
> fec_enet_free_buffers(ndev);
>
> Issue happend with 5.10.83 and should also also happen with current master.

It's fine for me, please see if anyone else has some comments. If not, please cook a formal patch, thanks.

Best Regards,
Joakim Zhang