[PATCH] dp83640: don't recieve time stamps twice

[PATCH] dp83640: don't recieve time stamps twice

[Dan Carpenter]

This patch is prompted by a static checker warning about a potential
use after free.  The concern is that netif_rx_ni() can free "skb" and we
call it twice.

When I look at the commit that added this, it looks like some stray
lines were added accidentally.  It doesn't make sense to me that we
would recieve the same data two times.  I asked the author but never
recieved a response.

I can't test this code, but I'm pretty sure my patch is correct.

Fixes: 4b063258ab93 ("dp83640: Delay scheduled work.")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index e2460a57e4b1..ed0d10f54f26 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1438,8 +1438,6 @@ static bool dp83640_rxtstamp(struct phy_device *phydev,
 		skb_info->tmo = jiffies + SKB_TIMESTAMP_TIMEOUT;
 		skb_queue_tail(&dp83640->rx_queue, skb);
 		schedule_delayed_work(&dp83640->ts_work, SKB_TIMESTAMP_TIMEOUT);
-	} else {
-		netif_rx_ni(skb);
 	}
 
 	return true;

Re: [PATCH] dp83640: don't recieve time stamps twice

[Richard Cochran]

On Tue, Apr 18, 2017 at 10:14:26PM +0300, Dan Carpenter wrote:
> This patch is prompted by a static checker warning about a potential
> use after free.  The concern is that netif_rx_ni() can free "skb" and we
> call it twice.

Right, the code already calls netif_rx_ni() in the list_for_each_safe()
loop just above, in the case that the shhwtstamps pointer has been set.
 
> When I look at the commit that added this, it looks like some stray
> lines were added accidentally.  It doesn't make sense to me that we
> would recieve the same data two times.  I asked the author but never
> recieved a response.

Hm, maybe the intent was to move the call to netif_rx_ni() outside of
the spin_lock_irqsave() region (which how I had it before Stefan's
changes).

But calling netif_rx_ni() twice is clearly wrong.

Thanks,
Richard

Re: [PATCH] dp83640: don't recieve time stamps twice

[Sørensen, Stefan]

T24gVHVlLCAyMDE3LTA0LTE4IGF0IDIyOjE0ICswMzAwLCBEYW4gQ2FycGVudGVyIHdyb3RlOg0K
PiBUaGlzIHBhdGNoIGlzIHByb21wdGVkIGJ5IGEgc3RhdGljIGNoZWNrZXIgd2FybmluZyBhYm91
dCBhIHBvdGVudGlhbA0KPiB1c2UgYWZ0ZXIgZnJlZS7CoMKgVGhlIGNvbmNlcm4gaXMgdGhhdCBu
ZXRpZl9yeF9uaSgpIGNhbiBmcmVlICJza2IiIGFuZA0KPiB3ZQ0KPiBjYWxsIGl0IHR3aWNlLg0K
PiANCj4gV2hlbiBJIGxvb2sgYXQgdGhlIGNvbW1pdCB0aGF0IGFkZGVkIHRoaXMsIGl0IGxvb2tz
IGxpa2Ugc29tZSBzdHJheQ0KPiBsaW5lcyB3ZXJlIGFkZGVkIGFjY2lkZW50YWxseS7CoMKgSXQg
ZG9lc24ndCBtYWtlIHNlbnNlIHRvIG1lIHRoYXQgd2UNCj4gd291bGQgcmVjaWV2ZSB0aGUgc2Ft
ZSBkYXRhIHR3byB0aW1lcy7CoMKgSSBhc2tlZCB0aGUgYXV0aG9yIGJ1dCBuZXZlcg0KPiByZWNp
ZXZlZCBhIHJlc3BvbnNlLg0KPiANCj4gSSBjYW4ndCB0ZXN0IHRoaXMgY29kZSwgYnV0IEknbSBw
cmV0dHkgc3VyZSBteSBwYXRjaCBpcyBjb3JyZWN0Lg0KDQpTb3JyeSBmb3Igbm90IGdldHRpbmcg
YmFjayBlYXJsaWVyLCBJIGhhdmUgc3dhbXBlZCB3aXRoIG90aGVyIHN0dWZmLg0KDQpZb3UgYXJl
IGNvcnJlY3QgdGhhdCB0aGVzZSBsaW5lcyB3YXMgYWRkZWQgYWNjaWRlbnRhbGx5Lg0KDQpBY2tl
ZC1ieTogU3RlZmFuIFPDuHJlbnNlbiA8c3RlZmFuLnNvcmVuc2VuQHNwZWN0cmFsaW5rLmNvbT4N
Cg0KPiANCj4gRml4ZXM6IDRiMDYzMjU4YWI5MyAoImRwODM2NDA6IERlbGF5IHNjaGVkdWxlZCB3
b3JrLiIpDQo+IFNpZ25lZC1vZmYtYnk6IERhbiBDYXJwZW50ZXIgPGRhbi5jYXJwZW50ZXJAb3Jh
Y2xlLmNvbT4NCj4gDQo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL25ldC9waHkvZHA4MzY0MC5jIGIv
ZHJpdmVycy9uZXQvcGh5L2RwODM2NDAuYw0KPiBpbmRleCBlMjQ2MGE1N2U0YjEuLmVkMGQxMGY1
NGYyNiAxMDA2NDQNCj4gLS0tIGEvZHJpdmVycy9uZXQvcGh5L2RwODM2NDAuYw0KPiArKysgYi9k
cml2ZXJzL25ldC9waHkvZHA4MzY0MC5jDQo+IEBAIC0xNDM4LDggKzE0MzgsNiBAQCBzdGF0aWMg
Ym9vbCBkcDgzNjQwX3J4dHN0YW1wKHN0cnVjdCBwaHlfZGV2aWNlDQo+ICpwaHlkZXYsDQo+IMKg
CQlza2JfaW5mby0+dG1vID0gamlmZmllcyArIFNLQl9USU1FU1RBTVBfVElNRU9VVDsNCj4gwqAJ
CXNrYl9xdWV1ZV90YWlsKCZkcDgzNjQwLT5yeF9xdWV1ZSwgc2tiKTsNCj4gwqAJCXNjaGVkdWxl
X2RlbGF5ZWRfd29yaygmZHA4MzY0MC0+dHNfd29yaywNCj4gU0tCX1RJTUVTVEFNUF9USU1FT1VU
KTsNCj4gLQl9IGVsc2Ugew0KPiAtCQluZXRpZl9yeF9uaShza2IpOw0KPiDCoAl9DQo+IMKgDQo+
IMKgCXJldHVybiB0cnVlOw=

Re: [PATCH] dp83640: don't recieve time stamps twice

[Richard Cochran]

On Wed, Apr 19, 2017 at 09:16:56AM +0000, Sørensen, Stefan wrote:
> You are correct that these lines was added accidentally.

Can we please fix this in another way?  There is no need to hold the
spin lock during the callback into the networking stack.  Instead, how
about the following diff, which also fixes the other call site...

Thanks,
Richard

---
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index e2460a5..593e533 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -874,14 +874,15 @@ static void decode_rxts(struct dp83640_private *dp83640,
 			shhwtstamps = skb_hwtstamps(skb);
 			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 			shhwtstamps->hwtstamp = ns_to_ktime(rxts->ns);
-			netif_rx_ni(skb);
 			list_add(&rxts->list, &dp83640->rxpool);
 			break;
 		}
 	}
 	spin_unlock(&dp83640->rx_queue.lock);
 
-	if (!shhwtstamps)
+	if (shhwtstamps)
+		netif_rx_ni(skb);
+	else
 		list_add_tail(&rxts->list, &dp83640->rxts);
 out:
 	spin_unlock_irqrestore(&dp83640->rx_lock, flags);
@@ -1425,7 +1426,6 @@ static bool dp83640_rxtstamp(struct phy_device *phydev,
 			shhwtstamps = skb_hwtstamps(skb);
 			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 			shhwtstamps->hwtstamp = ns_to_ktime(rxts->ns);
-			netif_rx_ni(skb);
 			list_del_init(&rxts->list);
 			list_add(&rxts->list, &dp83640->rxpool);
 			break;
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dp83640: don't recieve time stamps twice

[Dan Carpenter]

On Wed, Apr 19, 2017 at 12:31:35PM +0200, Richard Cochran wrote:
> On Wed, Apr 19, 2017 at 09:16:56AM +0000, Sørensen, Stefan wrote:
> > You are correct that these lines was added accidentally.
> 
> Can we please fix this in another way?  There is no need to hold the
> spin lock during the callback into the networking stack.  Instead, how
> about the following diff, which also fixes the other call site...
> 

I'm fine with this.  I hated my changelog, as well because it sounds
like guess work.  Could you send it with a:

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dp83640: don't recieve time stamps twice

[Sørensen, Stefan]

T24gV2VkLCAyMDE3LTA0LTE5IGF0IDEyOjMxICswMjAwLCBSaWNoYXJkIENvY2hyYW4gd3JvdGU6
DQo+IENhbiB3ZSBwbGVhc2UgZml4IHRoaXMgaW4gYW5vdGhlciB3YXk/wqDCoFRoZXJlIGlzIG5v
IG5lZWQgdG8gaG9sZCB0aGUNCj4gc3BpbiBsb2NrIGR1cmluZyB0aGUgY2FsbGJhY2sgaW50byB0
aGUgbmV0d29ya2luZyBzdGFjay7CoMKgSW5zdGVhZCwNCj4gaG93IGFib3V0IHRoZSBmb2xsb3dp
bmcgZGlmZiwgd2hpY2ggYWxzbyBmaXhlcyB0aGUgb3RoZXIgY2FsbCBzaXRlLi4uDQoNCg0KSSBh
Z3JlZSB0aGF0IHRoaXMgaXMgYSBiZXR0ZXIgd2F5IHRvIGZpeCBpdCAtIEkgdGhpbmsgdGhhdCB0
aGlzIHdhcyB0aGUNCnB1cnBvc2Ugb2YgdGhlIHBhdGNoIHRoYXQgZ290IGhhbGZ3YXkgbWl4ZWQg
aW4uDQoNCkBAIC04NzQsMTQgKzg3NCwxNSBAQCBzdGF0aWMgdm9pZCBkZWNvZGVfcnh0cyhzdHJ1
Y3QgZHA4MzY0MF9wcml2YXRlDQo+ICpkcDgzNjQwLA0KPiDCoAkJCXNoaHd0c3RhbXBzID0gc2ti
X2h3dHN0YW1wcyhza2IpOw0KPiDCoAkJCW1lbXNldChzaGh3dHN0YW1wcywgMCwNCj4gc2l6ZW9m
KCpzaGh3dHN0YW1wcykpOw0KPiDCoAkJCXNoaHd0c3RhbXBzLT5od3RzdGFtcCA9IG5zX3RvX2t0
aW1lKHJ4dHMtDQo+ID5ucyk7DQo+IC0JCQluZXRpZl9yeF9uaShza2IpOw0KPiDCoAkJCWxpc3Rf
YWRkKCZyeHRzLT5saXN0LCAmZHA4MzY0MC0+cnhwb29sKTsNCj4gwqAJCQlicmVhazsNCj4gwqAJ
CX0NCj4gwqAJfQ0KPiDCoAlzcGluX3VubG9jaygmZHA4MzY0MC0+cnhfcXVldWUubG9jayk7DQo+
IMKgDQo+IC0JaWYgKCFzaGh3dHN0YW1wcykNCj4gKwlpZiAoc2hod3RzdGFtcHMpDQo+ICsJCW5l
dGlmX3J4X25pKHNrYik7DQo+ICsJZWxzZQ0KPiDCoAkJbGlzdF9hZGRfdGFpbCgmcnh0cy0+bGlz
dCwgJmRwODM2NDAtPnJ4dHMpOw0KPiDCoG91dDoNCj4gwqAJc3Bpbl91bmxvY2tfaXJxcmVzdG9y
ZSgmZHA4MzY0MC0+cnhfbG9jaywgZmxhZ3MpOw0KDQpIZXJlIHdlIHN0aWxsIGhvbGQgcnhfbG9j
ayB3aGlsZSBkdXJpbmcgdGhlIGNhbGxiYWNrLCB3b3VsZG4ndCBpdCBiZQ0KYmVuZWZpY2lhbCB0
byByZWxlYXNlIHRoYXQgZmlyc3Q/DQoNClN0ZWZhbg0K

Re: [PATCH] dp83640: don't recieve time stamps twice

[Richard Cochran]

On Wed, Apr 19, 2017 at 11:28:34AM +0000, Sørensen, Stefan wrote:
> Here we still hold rx_lock while during the callback, wouldn't it be
> beneficial to release that first?

Yes, your are right.

Do you want to post a fix?  If not, I will, but later on this week.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dp83640: don't recieve time stamps twice

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Wed, 19 Apr 2017 12:31:35 +0200

> On Wed, Apr 19, 2017 at 09:16:56AM +0000, Sørensen, Stefan wrote:
>> You are correct that these lines was added accidentally.
> 
> Can we please fix this in another way?  There is no need to hold the
> spin lock during the callback into the networking stack.  Instead, how
> about the following diff, which also fixes the other call site...

Oops, I read this too late.

I already applied and pushed out Dan's fix.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dp83640: don't recieve time stamps twice

[Richard Cochran]

On Thu, Apr 20, 2017 at 04:02:27PM -0400, David Miller wrote:
> Oops, I read this too late.
> 
> I already applied and pushed out Dan's fix.

No problem, i'll submit a patch on top of that to move the netif calls
out of the spin-locked regions.

Thanks,
Richard