Currently the tid mask covers the first 4 bits of iwlagn_tx_resp::ra_tid,
which gives 16 possible values for tid.
This is problematic because IWL_MAX_TID_COUNT is 8, so indexing
iwl_priv::tid_data can go very wrong.
With UBSAN I can it happening while establishing the first connection
after module load.
[ 272.143440] UBSAN: Undefined behaviour in drivers/net/wireless/intel/iwlwifi/dvm/tx.c:777:32
[ 272.143447] index 8 is out of range for type 'iwl_tid_data [8]'
[ 272.143457] CPU: 0 PID: 4605 Comm: irq/32-iwlwifi Not tainted 4.12.0-dirty #2
[ 272.143460] Hardware name: Hewlett-Packard HP EliteBook 2560p/162B, BIOS 68SSU Ver. F.02 07/26/2011
[ 272.143462] Call Trace:
[ 272.143472] dump_stack+0x9c/0x10b
[ 272.143477] ? _atomic_dec_and_lock+0x285/0x285
[ 272.143486] ubsan_epilogue+0xd/0x4e
[ 272.143493] __ubsan_handle_out_of_bounds+0xef/0x118
[ 272.143498] ? __ubsan_handle_shift_out_of_bounds+0x221/0x221
[ 272.143519] ? iwl_trans_pcie_reclaim+0x153/0xc90 [iwlwifi]
[ 272.143539] iwlagn_check_ratid_empty+0x337/0x410 [iwldvm]
[ 272.143556] ? iwl_hcmd_names_cmp+0x2f/0x60 [iwlwifi]
[ 272.143571] iwlagn_rx_reply_tx+0x8a4/0x1820 [iwldvm]
Signed-off-by: Seraphime Kirkovski <[email protected]>
---
I'm currently running this patch on my machines and I have wifi.
The patch presumes а cleanup patch, I sent yesterday:
https://www.spinics.net/lists/kernel/msg2526314.html
drivers/net/wireless/intel/iwlwifi/dvm/commands.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/commands.h b/drivers/net/wireless/intel/iwlwifi/dvm/commands.h
index 37d2ba5ae852..e5994df9ea4c 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/commands.h
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/commands.h
@@ -1448,7 +1448,7 @@ struct agg_tx_status {
*/
/* refer to ra_tid */
#define IWLAGN_TX_RES_TID_POS 0
-#define IWLAGN_TX_RES_TID_MSK 0x0f
+#define IWLAGN_TX_RES_TID_MSK 0x07
#define IWLAGN_TX_RES_RA_POS 4
#define IWLAGN_TX_RES_RA_MSK 0xf0
--
2.11.0
On Thu, Jun 08, 2017 at 06:31:01AM +0000, Grumbach, Emmanuel wrote:
> Hi,
Hi,
> True, OTOH we need tid to be 8 sometimes. We *just* need to make sure
> that we don't index tid_data with this. Hence I think the proper fix is:
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/tx.c b/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
> index 06ac3f1..16a8646 100644
> --- a/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
> +++ b/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
> @@ -1190,11 +1190,11 @@ void iwlagn_rx_reply_tx(struct iwl_priv *priv, struct iwl_rx_cmd_buffer *rxb)
> next_reclaimed;
> IWL_DEBUG_TX_REPLY(priv, "Next reclaimed packet:%d\n",
> next_reclaimed);
> + iwlagn_check_ratid_empty(priv, sta_id, tid);
> }
>
> iwl_trans_reclaim(priv->trans, txq_id, ssn, &skbs);
>
> - iwlagn_check_ratid_empty(priv, sta_id, tid);
> freed = 0;
>
> /* process frames */
I can confirm it works. You can add my Tested-By.
Thanks,
Seraph
SGksDQoNCj4gU3ViamVjdDogW2xpbnV4d2lmaV0gW1BBVENIXSBuZXQ6IHdpcmVsZXNzOiBpbnRl
bDogaXdsd2lmaTogZHZtOiBmaXggdGlkIG1hc2sNCj4gDQo+IEN1cnJlbnRseSB0aGUgdGlkIG1h
c2sgY292ZXJzIHRoZSBmaXJzdCA0IGJpdHMgb2YgaXdsYWduX3R4X3Jlc3A6OnJhX3RpZCwNCj4g
d2hpY2ggZ2l2ZXMgMTYgcG9zc2libGUgdmFsdWVzIGZvciB0aWQuDQo+IFRoaXMgaXMgcHJvYmxl
bWF0aWMgYmVjYXVzZSBJV0xfTUFYX1RJRF9DT1VOVCBpcyA4LCBzbyBpbmRleGluZw0KPiBpd2xf
cHJpdjo6dGlkX2RhdGEgY2FuIGdvIHZlcnkgd3JvbmcuDQoNClRydWUsIE9UT0ggd2UgbmVlZCB0
aWQgdG8gYmUgOCBzb21ldGltZXMuIFdlICpqdXN0KiBuZWVkIHRvIG1ha2Ugc3VyZQ0KdGhhdCB3
ZSBkb24ndCBpbmRleCB0aWRfZGF0YSB3aXRoIHRoaXMuIEhlbmNlIEkgdGhpbmsgdGhlIHByb3Bl
ciBmaXggaXM6DQoNCmRpZmYgLS1naXQgYS9kcml2ZXJzL25ldC93aXJlbGVzcy9pbnRlbC9pd2x3
aWZpL2R2bS90eC5jIGIvZHJpdmVycy9uZXQvd2lyZWxlc3MvaW50ZWwvaXdsd2lmaS9kdm0vdHgu
Yw0KaW5kZXggMDZhYzNmMS4uMTZhODY0NiAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvbmV0L3dpcmVs
ZXNzL2ludGVsL2l3bHdpZmkvZHZtL3R4LmMNCisrKyBiL2RyaXZlcnMvbmV0L3dpcmVsZXNzL2lu
dGVsL2l3bHdpZmkvZHZtL3R4LmMNCkBAIC0xMTkwLDExICsxMTkwLDExIEBAIHZvaWQgaXdsYWdu
X3J4X3JlcGx5X3R4KHN0cnVjdCBpd2xfcHJpdiAqcHJpdiwgc3RydWN0IGl3bF9yeF9jbWRfYnVm
ZmVyICpyeGIpDQogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIG5leHRfcmVjbGFpbWVk
Ow0KICAgICAgICAgICAgICAgICAgICAgICAgSVdMX0RFQlVHX1RYX1JFUExZKHByaXYsICJOZXh0
IHJlY2xhaW1lZCBwYWNrZXQ6JWRcbiIsDQogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgIG5leHRfcmVjbGFpbWVkKTsNCisgICAgICAgICAgICAgICAgICAg
ICAgIGl3bGFnbl9jaGVja19yYXRpZF9lbXB0eShwcml2LCBzdGFfaWQsIHRpZCk7DQogICAgICAg
ICAgICAgICAgfQ0KDQogICAgICAgICAgICAgICAgaXdsX3RyYW5zX3JlY2xhaW0ocHJpdi0+dHJh
bnMsIHR4cV9pZCwgc3NuLCAmc2ticyk7DQoNCi0gICAgICAgICAgICAgICBpd2xhZ25fY2hlY2tf
cmF0aWRfZW1wdHkocHJpdiwgc3RhX2lkLCB0aWQpOw0KICAgICAgICAgICAgICAgIGZyZWVkID0g
MDsNCg0KICAgICAgICAgICAgICAgIC8qIHByb2Nlc3MgZnJhbWVzICovDQoNCg0KQ2xlYXJseSBj
YWxsaW5nIGl3bGFnbl9jaGVja19yYXRpZF9lbXB0eSB3aXRoIHRpZCA9IElXTF9USURfTk9OX1FP
UyBpcyBhIGJhZCBpZGVhLg0KDQo+IA0KPiBXaXRoIFVCU0FOIEkgY2FuIGl0IGhhcHBlbmluZyB3
aGlsZSBlc3RhYmxpc2hpbmcgdGhlIGZpcnN0IGNvbm5lY3Rpb24NCj4gYWZ0ZXIgbW9kdWxlIGxv
YWQuDQo+IA0KPiBbICAyNzIuMTQzNDQwXSBVQlNBTjogVW5kZWZpbmVkIGJlaGF2aW91ciBpbg0K
PiBkcml2ZXJzL25ldC93aXJlbGVzcy9pbnRlbC9pd2x3aWZpL2R2bS90eC5jOjc3NzozMg0KPiBb
ICAyNzIuMTQzNDQ3XSBpbmRleCA4IGlzIG91dCBvZiByYW5nZSBmb3IgdHlwZSAnaXdsX3RpZF9k
YXRhIFs4XScNCj4gWyAgMjcyLjE0MzQ1N10gQ1BVOiAwIFBJRDogNDYwNSBDb21tOiBpcnEvMzIt
aXdsd2lmaSBOb3QgdGFpbnRlZCA0LjEyLjAtZGlydHkNCj4gIzINCj4gWyAgMjcyLjE0MzQ2MF0g
SGFyZHdhcmUgbmFtZTogSGV3bGV0dC1QYWNrYXJkIEhQIEVsaXRlQm9vayAyNTYwcC8xNjJCLA0K
PiBCSU9TIDY4U1NVIFZlci4gRi4wMiAwNy8yNi8yMDExDQo+IFsgIDI3Mi4xNDM0NjJdIENhbGwg
VHJhY2U6DQo+IFsgIDI3Mi4xNDM0NzJdICBkdW1wX3N0YWNrKzB4OWMvMHgxMGINCj4gWyAgMjcy
LjE0MzQ3N10gID8gX2F0b21pY19kZWNfYW5kX2xvY2srMHgyODUvMHgyODUNCj4gWyAgMjcyLjE0
MzQ4Nl0gIHVic2FuX2VwaWxvZ3VlKzB4ZC8weDRlDQo+IFsgIDI3Mi4xNDM0OTNdICBfX3Vic2Fu
X2hhbmRsZV9vdXRfb2ZfYm91bmRzKzB4ZWYvMHgxMTgNCj4gWyAgMjcyLjE0MzQ5OF0gID8gX191
YnNhbl9oYW5kbGVfc2hpZnRfb3V0X29mX2JvdW5kcysweDIyMS8weDIyMQ0KPiBbICAyNzIuMTQz
NTE5XSAgPyBpd2xfdHJhbnNfcGNpZV9yZWNsYWltKzB4MTUzLzB4YzkwIFtpd2x3aWZpXQ0KPiBb
ICAyNzIuMTQzNTM5XSAgaXdsYWduX2NoZWNrX3JhdGlkX2VtcHR5KzB4MzM3LzB4NDEwIFtpd2xk
dm1dDQo+IFsgIDI3Mi4xNDM1NTZdICA/IGl3bF9oY21kX25hbWVzX2NtcCsweDJmLzB4NjAgW2l3
bHdpZmldDQo+IFsgIDI3Mi4xNDM1NzFdICBpd2xhZ25fcnhfcmVwbHlfdHgrMHg4YTQvMHgxODIw
IFtpd2xkdm1dDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBTZXJhcGhpbWUgS2lya292c2tpIDxraXJr
c2VyYXBoQGdtYWlsLmNvbT4NCj4gLS0tDQo+ICAgIEknbSBjdXJyZW50bHkgcnVubmluZyB0aGlz
IHBhdGNoIG9uIG15IG1hY2hpbmVzIGFuZCBJIGhhdmUgd2lmaS4NCj4gICAgVGhlIHBhdGNoIHBy
ZXN1bWVzINCwIGNsZWFudXAgcGF0Y2gsIEkgc2VudCB5ZXN0ZXJkYXk6DQo+ICAgICAgIGh0dHBz
Oi8vd3d3LnNwaW5pY3MubmV0L2xpc3RzL2tlcm5lbC9tc2cyNTI2MzE0Lmh0bWwNCj4gDQo+ICBk
cml2ZXJzL25ldC93aXJlbGVzcy9pbnRlbC9pd2x3aWZpL2R2bS9jb21tYW5kcy5oIHwgMiArLQ0K
PiAgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pDQo+IA0KPiBk
aWZmIC0tZ2l0IGEvZHJpdmVycy9uZXQvd2lyZWxlc3MvaW50ZWwvaXdsd2lmaS9kdm0vY29tbWFu
ZHMuaA0KPiBiL2RyaXZlcnMvbmV0L3dpcmVsZXNzL2ludGVsL2l3bHdpZmkvZHZtL2NvbW1hbmRz
LmgNCj4gaW5kZXggMzdkMmJhNWFlODUyLi5lNTk5NGRmOWVhNGMgMTAwNjQ0DQo+IC0tLSBhL2Ry
aXZlcnMvbmV0L3dpcmVsZXNzL2ludGVsL2l3bHdpZmkvZHZtL2NvbW1hbmRzLmgNCj4gKysrIGIv
ZHJpdmVycy9uZXQvd2lyZWxlc3MvaW50ZWwvaXdsd2lmaS9kdm0vY29tbWFuZHMuaA0KPiBAQCAt
MTQ0OCw3ICsxNDQ4LDcgQEAgc3RydWN0IGFnZ190eF9zdGF0dXMgew0KPiAgICovDQo+ICAvKiBy
ZWZlciB0byByYV90aWQgKi8NCj4gICNkZWZpbmUgSVdMQUdOX1RYX1JFU19USURfUE9TCTANCj4g
LSNkZWZpbmUgSVdMQUdOX1RYX1JFU19USURfTVNLCTB4MGYNCj4gKyNkZWZpbmUgSVdMQUdOX1RY
X1JFU19USURfTVNLCTB4MDcNCj4gICNkZWZpbmUgSVdMQUdOX1RYX1JFU19SQV9QT1MJNA0KPiAg
I2RlZmluZSBJV0xBR05fVFhfUkVTX1JBX01TSwkweGYwDQo+IA0KPiAtLQ0KPiAyLjExLjANCj4g
DQo+IC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCj4gbGludXh3aWZpQGVj
bGlzdHMuaW50ZWwuY29tDQo+IGh0dHBzOi8vZWNsaXN0cy5pbnRlbC5jb20vc3ltcGEvaW5mby9s
aW51eHdpZmkNCj4gVW5zdWJzY3JpYmUgYnkgc2VuZGluZyBlbWFpbCB0byBzeW1wYUBlY2xpc3Rz
LmludGVsLmNvbSB3aXRoIHN1YmplY3QNCj4gIlVuc3Vic2NyaWJlIGxpbnV4d2lmaSINCg==
>
> On Thu, Jun 08, 2017 at 06:31:01AM +0000, Grumbach, Emmanuel wrote:
> > Hi,
>
> Hi,
>
> > True, OTOH we need tid to be 8 sometimes. We *just* need to make sure
> > that we don't index tid_data with this. Hence I think the proper fix is:
> >
> > diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
> > b/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
> > index 06ac3f1..16a8646 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
> > @@ -1190,11 +1190,11 @@ void iwlagn_rx_reply_tx(struct iwl_priv *priv,
> struct iwl_rx_cmd_buffer *rxb)
> > next_reclaimed;
> > IWL_DEBUG_TX_REPLY(priv, "Next reclaimed packet:%d\n",
> > next_reclaimed);
> > + iwlagn_check_ratid_empty(priv, sta_id, tid);
> > }
> >
> > iwl_trans_reclaim(priv->trans, txq_id, ssn, &skbs);
> >
> > - iwlagn_check_ratid_empty(priv, sta_id, tid);
> > freed = 0;
> >
> > /* process frames */
>
> I can confirm it works. You can add my Tested-By.
Patch in review in our internal tree. It'll be upstreamed through the regular process.
Thanks for your report and debug work.