linux-kernel - Re: [PATCH 1 of 1]: musb: fixed a potential NULL pointer dereference.

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]

Message-Id: <D56478EB-6936-4DFC-94F9-2A9A3CC217F1@goldelico.com>
Date:	Wed, 12 Feb 2014 16:35:12 +0100
From:	"Dr. H. Nikolaus Schaller" <hns@...delico.com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Felipe Balbi <balbi@...com>, linux-usb@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>,
	Marek Belisko <marek@...delico.com>
Subject: Re: [PATCH 1 of 1]: musb: fixed a potential NULL pointer dereference.


Am 12.02.2014 um 15:29 schrieb Greg Kroah-Hartman:

> On Wed, Feb 12, 2014 at 11:08:22AM +0100, Dr. H. Nikolaus Schaller wrote:
>> fixed a potential NULL pointer dereference.
>> 
>>    Rationale:
>>    this is the only location in the musb driver where the
>>    otg->gadget pointer is dereferenced. Assuming that it
>>    is never NULL is not only potentially unsafe but was
>>    observed in the wild on a GTA04 (OMAP3/TPS65950 based
>>    board) when trying to boot a device tree based 3.14-rc2
>>    kernel with USB cable plugged in.
>> 
>>    DT boot appears to modify the order in which components
>>    (gadget driver) are loaded and linked and therefore
>>    an early musb interrupt triggers with a NULL gadget
>>    pointer ending in a kernel panic.
>> 
>>    Since a non-existing gadget can never be "active" we
>>    simply use a 0 value for musb->is_active.
>> 
>>    Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
> 
> Please don't attach patches, our tools don't like them at all.

Sorry. That was by accident. Here is the patch appended.

> 
> And shouldn't we fix the root problem here, not just gloss over the fact
> that this pointer is NULL at this point in time?  Fixing the real issue
> should be the correct solution, right?

Well,
I must admit that I have no idea what the root cause could be 
(because I neither know the musb nor the gadget subsystems)
or if it is intended and just something in the initialization
sequence has changed.

I have done one more test and it appears to be in a non-device-tree
3.14-rc1 as well. We did not see it in a 3.12.7 kernel on the same hardware.

> 
> thanks,
> 
> greg k-h


BR,
Nikolaus Schaller

---
 drivers/usb/musb/musb_core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index df3f65d..f68afef 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -653,7 +653,8 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
 				break;
 		case OTG_STATE_B_PERIPHERAL:
 			musb_g_suspend(musb);
-			musb->is_active = otg->gadget->b_hnp_enable;
+			musb->is_active =
+				otg->gadget ? otg->gadget->b_hnp_enable : 0;
 			if (musb->is_active) {
 				musb->xceiv->state = OTG_STATE_B_WAIT_ACON;
 				dev_dbg(musb->controller, "HNP: Setting timer for b_ase0_brst\n");
-- 
1.7.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/