Skip to contentSkip to conversation

watchdog: pcwd_usb: fix NULL-deref at probe

PATCH In review Johan Hovold 3 participants 11 messages Mar 13, 2017 – May 14, 2017

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 watchdog: pcwd_usb: fix NULL-deref at probe

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
---
 drivers/watchdog/pcwd_usb.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/watchdog/pcwd_usb.c +3 -0
index 99ebf6ea3de6..5615f4013924 100644
@@ -630,6 +630,9 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 		return -ENODEV;
 	}
 
+	if (iface_desc->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	/* check out the endpoint: it has to be Interrupt & IN */
 	endpoint = &iface_desc->endpoint[0].desc;
 
Signature · 1 line
2.12.0
maintainer
#1 lore raw
Quoted 3 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Is this theory or was it actually observed ?

Thanks,
Guenter

Quoted 22 lines
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
---
 drivers/watchdog/pcwd_usb.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/watchdog/pcwd_usb.c +3 -0
index 99ebf6ea3de6..5615f4013924 100644
@@ -630,6 +630,9 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 		return -ENODEV;
 	}
 
+	if (iface_desc->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	/* check out the endpoint: it has to be Interrupt & IN */
 	endpoint = &iface_desc->endpoint[0].desc;
 
#2 lore raw

[ Adding linux-usb which I forgot to CC for this one ]

Quoted 6 lines · Guenter Roeck
Quoted 3 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Is this theory or was it actually observed ?

This was found through inspection, but creating a USB device to crash a host with this driver enabled is easily done.

Quoted 15 lines
Quoted 15 lines
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
---
 drivers/watchdog/pcwd_usb.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/watchdog/pcwd_usb.c +0 -0
index 99ebf6ea3de6..5615f4013924 100644
@@ -630,6 +630,9 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 		return -ENODEV;
 	}
 
Quoted 7 lines

drivers/watchdog/pcwd_usb.c static int usb_pcwd_probe(struct usb_interface *interface, · line 638

Quoted 7 lines
drivers/watchdog/pcwd_usb.c +3 -0
+	if (iface_desc->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	/* check out the endpoint: it has to be Interrupt & IN */
 	endpoint = &iface_desc->endpoint[0].desc;
 

Johan

maintainer
#3 lore raw
Quoted 4 lines · Johan Hovold

[ Adding linux-usb which I forgot to CC for this one ]

Quoted 1 line · Guenter Roeck

On 03/13/2017 05:49 AM, Johan Hovold wrote:

Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Is this theory or was it actually observed ?

This was found through inspection, but creating a USB device to crash a host with this driver enabled is easily done.

Ok, makes sense. I see other drivers doing a similar check.

Guenter

Quoted 24 lines
Quoted 15 lines
Quoted 15 lines
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
---
 drivers/watchdog/pcwd_usb.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/watchdog/pcwd_usb.c +0 -0
index 99ebf6ea3de6..5615f4013924 100644
@@ -630,6 +630,9 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 		return -ENODEV;
 	}
 
Quoted 7 lines

drivers/watchdog/pcwd_usb.c static int usb_pcwd_probe(struct usb_interface *interface, · line 638

Quoted 7 lines
drivers/watchdog/pcwd_usb.c +3 -0
+	if (iface_desc->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	/* check out the endpoint: it has to be Interrupt & IN */
 	endpoint = &iface_desc->endpoint[0].desc;
 

Johan

maintainer Reviewed-by on v1 1/1
#4 lore raw
In reply to Johan Hovold
Quoted 6 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>

Note that I dropped Cc: stable from my reply since it is not appropriate at this time.

Guenter

Quoted 21 lines
---
 drivers/watchdog/pcwd_usb.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/watchdog/pcwd_usb.c +3 -0
index 99ebf6ea3de6..5615f4013924 100644
@@ -630,6 +630,9 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 		return -ENODEV;
 	}
 
+	if (iface_desc->desc.bNumEndpoints < 1)
+		return -ENODEV;
+
 	/* check out the endpoint: it has to be Interrupt & IN */
 	endpoint = &iface_desc->endpoint[0].desc;
 
Signature · 1 line
2.12.0
#5 lore raw
Quoted 9 lines · Guenter Roeck
Quoted 6 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>

Any progress on this one? I noticed you merged it to both the fixes and next branches in your staging tree, Guenter (but it does not show up in linux-next). Will you be sending it on to Linus?

Thanks,
Johan

maintainer
#6 lore raw
Quoted 9 lines · Johan Hovold
Quoted 8 lines · Guenter Roeck
Quoted 6 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
Quoted 6 lines
Quoted 1 line

Any progress on this one? I noticed you merged it to both the fixes and next branches in your staging tree, Guenter (but it does not show up in linux-next). Will you be sending it on to Linus?

Hi Johan,

my watchdog staging trees are inofficial and not in linux-next. Wim is working on setting up a new server which will provide the official staging tree.

I asked Wim to push the pending patches. I'll do it if he asks me.

Thanks,
Guenter

#7 lore raw

Hi Guenter and Wim,

Quoted 16 lines · Guenter Roeck
Quoted 9 lines · Johan Hovold
Quoted 8 lines · Guenter Roeck
Quoted 6 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
Quoted 5 lines
Quoted 1 line

Any progress on this one? I noticed you merged it to both the fixes and next branches in your staging tree, Guenter (but it does not show up in linux-next). Will you be sending it on to Linus?

Quoted 5 lines

my watchdog staging trees are inofficial and not in linux-next. Wim is working on setting up a new server which will provide the official staging tree.

I asked Wim to push the pending patches. I'll do it if he asks me.

I noticed Guenter's watchdog branch is now in next, but the patches for 4.12 are still not in mainline. Have you guys decided who will be sending them on to Linus this cycle?

Thanks,
Johan

maintainer
#8 lore raw
Quoted 23 lines · Johan Hovold

Hi Guenter and Wim,

Quoted 20 lines · Guenter Roeck
Quoted 9 lines · Johan Hovold
Quoted 8 lines · Guenter Roeck
Quoted 6 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
Quoted 5 lines
Quoted 1 line

Any progress on this one? I noticed you merged it to both the fixes and next branches in your staging tree, Guenter (but it does not show up in linux-next). Will you be sending it on to Linus?

my watchdog staging trees are inofficial and not in linux-next. Wim is working on setting up a new server which will provide the official staging tree.

Quoted 6 lines
Quoted 1 line

I asked Wim to push the pending patches. I'll do it if he asks me.

I noticed Guenter's watchdog branch is now in next, but the patches for 4.12 are still not in mainline. Have you guys decided who will be sending them on to Linus this cycle?

Good question. I had expected Wim to do it, since it is actually his repository which is in linux-next. But you are correct, it isn't upstream.

Wim ?

Thanks,
Guenter

#9 lore raw

All,

Quoted 24 lines · Johan Hovold
Quoted 23 lines

Hi Guenter and Wim,

Quoted 20 lines · Guenter Roeck
Quoted 9 lines · Johan Hovold
Quoted 8 lines · Guenter Roeck
Quoted 6 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
Quoted 5 lines
Quoted 1 line

Any progress on this one? I noticed you merged it to both the fixes and next branches in your staging tree, Guenter (but it does not show up in linux-next). Will you be sending it on to Linus?

my watchdog staging trees are inofficial and not in linux-next. Wim is working on setting up a new server which will provide the official staging tree.

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

I asked Wim to push the pending patches. I'll do it if he asks me.

I noticed Guenter's watchdog branch is now in next, but the patches for 4.12 are still not in mainline. Have you guys decided who will be sending them on to Linus this cycle?

Good question. I had expected Wim to do it, since it is actually his repository which is in linux-next. But you are correct, it isn't upstream.

Due to some issues, we (Linus, Guenter and myself) decided that Guenter will sent the pull request this time (which he allready did).

Kind regards,
Wim.

#10 lore raw

All,

Quoted 26 lines
Quoted 26 lines · Johan Hovold
Quoted 25 lines

Hi Guenter and Wim,

Quoted 21 lines · Guenter Roeck
Quoted 9 lines · Johan Hovold
Quoted 8 lines · Guenter Roeck
Quoted 6 lines · Johan Hovold

Make sure to check the number of endpoints to avoid dereferencing a NULL-pointer should a malicious device lack endpoints.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc stable <[email protected]>
Quoted 5 lines
Quoted 1 line

Any progress on this one? I noticed you merged it to both the fixes and next branches in your staging tree, Guenter (but it does not show up in linux-next). Will you be sending it on to Linus?

my watchdog staging trees are inofficial and not in linux-next. Wim is working on setting up a new server which will provide the official staging tree.

I asked Wim to push the pending patches. I'll do it if he asks me.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

I noticed Guenter's watchdog branch is now in next, but the patches for 4.12 are still not in mainline. Have you guys decided who will be sending them on to Linus this cycle?

Good question. I had expected Wim to do it, since it is actually his repository which is in linux-next. But you are correct, it isn't upstream.

Due to some issues, we (Linus, Guenter and myself) decided that Guenter will sent the pull request this time (which he allready did).

OK, we were to late. We will line up the fixes for rc2. Feature changes and new drivers will be for next merge window. My apologies to the people that have sent in these changes and drivers.

Kind regards,
Wim.