| 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
| ||
|
Message-ID: <57164383.6080103@ti.com>
Date: Tue, 19 Apr 2016 17:41:07 +0300
From: Grygorii Strashko <grygorii.strashko@...com>
To: Andrew Goodbody <andrew.goodbody@...brionix.com>,
<netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>
CC: <linux-kernel@...r.kernel.org>, <linux-omap@...r.kernel.org>,
<mugunthanvnm@...com>, <tony@...mide.com>
Subject: Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer
dereference with two PHYs
Hi,
On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
> Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> as below. Fix by maintaining a reference to each PHY node in slave
> struct instead of a single reference in the priv struct which was
> overwritten by the 2nd PHY.
David, Is it possible to drop prev version of this patch from linux-next
- it breaks boot on many TI boards with -next.
>
> [ 17.870933] Unable to handle kernel NULL pointer dereference at virtual address 00000180
> [ 17.879557] pgd = dc8bc000
> [ 17.882514] [00000180] *pgd=9c882831, *pte=00000000, *ppte=00000000
> [ 17.889213] Internal error: Oops: 17 [#1] ARM
> [ 17.893838] Modules linked in:
> [ 17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted 4.5.0-ge463dfb-dirty #11
> [ 17.904947] Hardware name: Cambrionix whippet
> [ 17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> [ 17.915339] PC is at phy_attached_print+0x18/0x8c
> [ 17.920339] LR is at phy_attached_info+0x14/0x18
> [ 17.925247] pc : [<c042baec>] lr : [<c042bb74>] psr: 600f0113
> [ 17.925247] sp : dc969cf8 ip : dc969d28 fp : dc969d18
> [ 17.937425] r10: dda7a400 r9 : 00000000 r8 : 00000000
> [ 17.942971] r7 : 00000001 r6 : ddb00480 r5 : ddb8cb34 r4 : 00000000
> [ 17.949898] r3 : c0954cc0 r2 : c09562b0 r1 : 00000000 r0 : 00000000
> [ 17.956829] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> [ 17.964401] Control: 10c5387d Table: 9c8bc019 DAC: 00000051
> [ 17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> [ 17.977059] Stack: (0xdc969cf8 to 0xdc96a000)
[...]
> [ 18.323956] [<c05e4cb8>] (inet_ioctl) from [<c055f5ac>] (sock_ioctl+0x15c/0x2d8)
> [ 18.331829] [<c055f450>] (sock_ioctl) from [<c010b388>] (do_vfs_ioctl+0x98/0x8d0)
> [ 18.339765] r7:00008914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> [ 18.345822] [<c010b2f0>] (do_vfs_ioctl) from [<c010bc34>] (SyS_ioctl+0x74/0x84)
> [ 18.353573] r10:00000000 r9:00000011 r8:beaeda20 r7:00008914 r6:dc8ab4c0 r5:dc8ab4c0
> [ 18.361924] r4:00000000
> [ 18.364653] [<c010bbc0>] (SyS_ioctl) from [<c00163e0>] (ret_fast_syscall+0x0/0x3c)
> [ 18.372682] r9:dc968000 r8:c00165e8 r7:00000036 r6:00000002 r5:00000011 r4:00000000
> [ 18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> [ 18.387580] ---[ end trace c80529466223f3f3 ]---
^ Could you make it shorter and drop timestamps, pls?
>
> Signed-off-by: Andrew Goodbody <andrew.goodbody@...brionix.com>
> ---
>
> v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt so it
> has data->slaves initialised first which is needed to calculate size
>
> drivers/net/ethernet/ti/cpsw.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 42fdfd4..e62909c 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -349,6 +349,7 @@ struct cpsw_slave {
> struct cpsw_slave_data *data;
> struct phy_device *phy;
> struct net_device *ndev;
> + struct device_node *phy_node;
> u32 port_vlan;
> u32 open_stat;
> };
> @@ -367,7 +368,6 @@ struct cpsw_priv {
> spinlock_t lock;
> struct platform_device *pdev;
> struct net_device *ndev;
> - struct device_node *phy_node;
> struct napi_struct napi_rx;
> struct napi_struct napi_tx;
> struct device *dev;
> @@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
> cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
> 1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
>
> - if (priv->phy_node)
> - slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
> + if (slave->phy_node)
> + slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
> &cpsw_adjust_link, 0, slave->data->phy_if);
> else
> slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> @@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> struct device_node *node = pdev->dev.of_node;
> struct device_node *slave_node;
> struct cpsw_platform_data *data = &priv->data;
> - int i = 0, ret;
> + int i, ret;
> u32 prop;
>
> if (!node)
> @@ -1958,6 +1958,14 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> }
> data->slaves = prop;
>
> + priv->slaves = devm_kzalloc(&pdev->dev,
> + sizeof(struct cpsw_slave) * data->slaves,
> + GFP_KERNEL);
> + if (!priv->slaves)
> + return -ENOMEM;
> + for (i = 0; i < data->slaves; i++)
> + priv->slaves[i].slave_num = i;
> +
> if (of_property_read_u32(node, "active_slave", &prop)) {
> dev_err(&pdev->dev, "Missing active_slave property in the DT.\n");
> return -EINVAL;
> @@ -2023,6 +2031,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> if (ret)
> dev_warn(&pdev->dev, "Doesn't have any child node\n");
>
> + i = 0;
> for_each_child_of_node(node, slave_node) {
> struct cpsw_slave_data *slave_data = data->slave_data + i;
> const void *mac_addr = NULL;
> @@ -2033,7 +2042,8 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> if (strcmp(slave_node->name, "slave"))
> continue;
>
> - priv->phy_node = of_parse_phandle(slave_node, "phy-handle", 0);
> + priv->slaves[i].phy_node =
> + of_parse_phandle(slave_node, "phy-handle", 0);
i++?
Ideally, the simplest way is to save phy_node in slave_data, but ...
(see comment below).
> parp = of_get_property(slave_node, "phy_id", &lenp);
> if (of_phy_is_fixed_link(slave_node)) {
> struct device_node *phy_node;
> @@ -2292,16 +2302,6 @@ static int cpsw_probe(struct platform_device *pdev)
>
> memcpy(ndev->dev_addr, priv->mac_addr, ETH_ALEN);
>
> - priv->slaves = devm_kzalloc(&pdev->dev,
> - sizeof(struct cpsw_slave) * data->slaves,
> - GFP_KERNEL);
> - if (!priv->slaves) {
> - ret = -ENOMEM;
> - goto clean_runtime_disable_ret;
> - }
I don't think you can move this out from here - it will break legacy boot :(
> - for (i = 0; i < data->slaves; i++)
> - priv->slaves[i].slave_num = i;
Personally, I see only one safe way to do it without big rework -
do second pass of DT parsing here to fill phy_node field.
> -
> priv->slaves[0].ndev = ndev;
> priv->emac_port = 0;
>
>
--
regards,
-grygorii
Powered by blists - more mailing lists