From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-wg0-f48.google.com ([74.125.82.48]:61357 "EHLO mail-wg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750798AbaDKH64 convert rfc822-to-8bit (ORCPT ); Fri, 11 Apr 2014 03:58:56 -0400 Received: by mail-wg0-f48.google.com with SMTP id l18so4995503wgh.31 for ; Fri, 11 Apr 2014 00:58:54 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <53461A8A.4030209@candelatech.com> <1397124355-6321-1-git-send-email-michal.kazior@tieto.com> <8738hkh193.fsf@kamboji.qca.qualcomm.com> Date: Fri, 11 Apr 2014 09:58:54 +0200 Message-ID: (sfid-20140411_095931_815076_39F4F962) Subject: Re: [PATCH] ath10k: double check bmi xfer pointers From: Michal Kazior To: Kalle Valo Cc: "ath10k@lists.infradead.org" , Ben Greear , linux-wireless Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 11 April 2014 07:47, Michal Kazior wrote: > On 11 April 2014 07:40, Kalle Valo wrote: >> Michal Kazior writes: >> >>> If for some reason copy engine ring buffer became >>> corrupt ath10k could crash the machine due to >>> invalid pointer dereference. It's very unlikely >>> but devices can never be fully trusted so verify >>> if the bmi xfer pointer read back from copy engine >>> matches the original pointer. >> >> The big question is why does this happen? Does this happen only with >> Ben's firmware or is it a more generic problem? > > I'll look more into this. Hmm.. After going through the code a few times I think the bug is actually something else. If the crash happened in complete() it means the completion structure wasn't set up properly. However it is always initialized in ath10k_pci_hif_exchange_bmi_msg() before proceeding. This means xfer pointer read back from ath10k_ce_completed_send_next() or ath10k_ce_completed_recv_next() is wrong. Since the pointer to it is kept on host getting wrong xfer means sw_index must be wrong. If I assume indexes are managed correctly (i.e. no overflows, locking is fine) then it is the entry the sw_index points to that is actually unexpected. This could happen if concurrent transfers occur on pipe 0 or 1 (used for bmi communication) during device bootup. This could be either a concurrent bmi transfer or a non-bmi buffer or an old bmi xfer data. The latter shouldn't be the case because ath10k_pci_hif_exchange_bmi_msg() cleans up after itself. Concurrent access doesn't seem to be the case either. I conclude the bug is not present in the vanilla ath10k code. TL;DR drop the patch, please Michał From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-wi0-x230.google.com ([2a00:1450:400c:c05::230]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WYWMt-0007xe-PO for ath10k@lists.infradead.org; Fri, 11 Apr 2014 07:59:24 +0000 Received: by mail-wi0-f176.google.com with SMTP id r20so555131wiv.15 for ; Fri, 11 Apr 2014 00:58:55 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <53461A8A.4030209@candelatech.com> <1397124355-6321-1-git-send-email-michal.kazior@tieto.com> <8738hkh193.fsf@kamboji.qca.qualcomm.com> Date: Fri, 11 Apr 2014 09:58:54 +0200 Message-ID: Subject: Re: [PATCH] ath10k: double check bmi xfer pointers From: Michal Kazior List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: Kalle Valo Cc: Ben Greear , linux-wireless , "ath10k@lists.infradead.org" T24gMTEgQXByaWwgMjAxNCAwNzo0NywgTWljaGFsIEthemlvciA8bWljaGFsLmthemlvckB0aWV0 by5jb20+IHdyb3RlOgo+IE9uIDExIEFwcmlsIDIwMTQgMDc6NDAsIEthbGxlIFZhbG8gPGt2YWxv QHFjYS5xdWFsY29tbS5jb20+IHdyb3RlOgo+PiBNaWNoYWwgS2F6aW9yIDxtaWNoYWwua2F6aW9y QHRpZXRvLmNvbT4gd3JpdGVzOgo+Pgo+Pj4gSWYgZm9yIHNvbWUgcmVhc29uIGNvcHkgZW5naW5l IHJpbmcgYnVmZmVyIGJlY2FtZQo+Pj4gY29ycnVwdCBhdGgxMGsgY291bGQgY3Jhc2ggdGhlIG1h Y2hpbmUgZHVlIHRvCj4+PiBpbnZhbGlkIHBvaW50ZXIgZGVyZWZlcmVuY2UuIEl0J3MgdmVyeSB1 bmxpa2VseQo+Pj4gYnV0IGRldmljZXMgY2FuIG5ldmVyIGJlIGZ1bGx5IHRydXN0ZWQgc28gdmVy aWZ5Cj4+PiBpZiB0aGUgYm1pIHhmZXIgcG9pbnRlciByZWFkIGJhY2sgZnJvbSBjb3B5IGVuZ2lu ZQo+Pj4gbWF0Y2hlcyB0aGUgb3JpZ2luYWwgcG9pbnRlci4KPj4KPj4gVGhlIGJpZyBxdWVzdGlv biBpcyB3aHkgZG9lcyB0aGlzIGhhcHBlbj8gRG9lcyB0aGlzIGhhcHBlbiBvbmx5IHdpdGgKPj4g QmVuJ3MgZmlybXdhcmUgb3IgaXMgaXQgYSBtb3JlIGdlbmVyaWMgcHJvYmxlbT8KPgo+IEknbGwg bG9vayBtb3JlIGludG8gdGhpcy4KCkhtbS4uIEFmdGVyIGdvaW5nIHRocm91Z2ggdGhlIGNvZGUg YSBmZXcgdGltZXMgSSB0aGluayB0aGUgYnVnIGlzCmFjdHVhbGx5IHNvbWV0aGluZyBlbHNlLgoK SWYgdGhlIGNyYXNoIGhhcHBlbmVkIGluIGNvbXBsZXRlKCkgaXQgbWVhbnMgdGhlIGNvbXBsZXRp b24gc3RydWN0dXJlCndhc24ndCBzZXQgdXAgcHJvcGVybHkuIEhvd2V2ZXIgaXQgaXMgYWx3YXlz IGluaXRpYWxpemVkIGluCmF0aDEwa19wY2lfaGlmX2V4Y2hhbmdlX2JtaV9tc2coKSBiZWZvcmUg cHJvY2VlZGluZy4gVGhpcyBtZWFucyB4ZmVyCnBvaW50ZXIgcmVhZCBiYWNrIGZyb20gYXRoMTBr X2NlX2NvbXBsZXRlZF9zZW5kX25leHQoKSBvcgphdGgxMGtfY2VfY29tcGxldGVkX3JlY3ZfbmV4 dCgpIGlzIHdyb25nLiBTaW5jZSB0aGUgcG9pbnRlciB0byBpdCBpcwprZXB0IG9uIGhvc3QgZ2V0 dGluZyB3cm9uZyB4ZmVyIG1lYW5zIHN3X2luZGV4IG11c3QgYmUgd3JvbmcuIElmIEkKYXNzdW1l IGluZGV4ZXMgYXJlIG1hbmFnZWQgY29ycmVjdGx5IChpLmUuIG5vIG92ZXJmbG93cywgbG9ja2lu ZyBpcwpmaW5lKSB0aGVuIGl0IGlzIHRoZSBlbnRyeSB0aGUgc3dfaW5kZXggcG9pbnRzIHRvIHRo YXQgaXMgYWN0dWFsbHkKdW5leHBlY3RlZC4KClRoaXMgY291bGQgaGFwcGVuIGlmIGNvbmN1cnJl bnQgdHJhbnNmZXJzIG9jY3VyIG9uIHBpcGUgMCBvciAxICh1c2VkCmZvciBibWkgY29tbXVuaWNh dGlvbikgZHVyaW5nIGRldmljZSBib290dXAuIFRoaXMgY291bGQgYmUgZWl0aGVyIGEKY29uY3Vy cmVudCBibWkgdHJhbnNmZXIgb3IgYSBub24tYm1pIGJ1ZmZlciBvciBhbiBvbGQgYm1pIHhmZXIg ZGF0YS4KVGhlIGxhdHRlciBzaG91bGRuJ3QgYmUgdGhlIGNhc2UgYmVjYXVzZQphdGgxMGtfcGNp X2hpZl9leGNoYW5nZV9ibWlfbXNnKCkgY2xlYW5zIHVwIGFmdGVyIGl0c2VsZi4gQ29uY3VycmVu dAphY2Nlc3MgZG9lc24ndCBzZWVtIHRvIGJlIHRoZSBjYXNlIGVpdGhlci4KCkkgY29uY2x1ZGUg dGhlIGJ1ZyBpcyBub3QgcHJlc2VudCBpbiB0aGUgdmFuaWxsYSBhdGgxMGsgY29kZS4KClRMO0RS IGRyb3AgdGhlIHBhdGNoLCBwbGVhc2UKCgpNaWNoYcWCCgpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwphdGgxMGsgbWFpbGluZyBsaXN0CmF0aDEwa0BsaXN0 cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGlu Zm8vYXRoMTBrCg==