Skip to contentSkip to conversation

drivers/firewire: fix use/leak of uninitialized stack memory in dispatch_ioctl()

PATCH New Stefan Richter 2 participants 4 messages Nov 11, 2014 3 messages not in the archive

Subsystems (2)
ABI/API Firewire
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20141111010340.3329bbd7@kant

A message that is not in the archive (sent off-list or not yet received): 20141111130143.2ff3d42e@kant

Stefan Richter

author
#3 lore raw

Adding Cc: linux-api and lkml, quoting parent message in full

Quoted 50 lines · On Nov 11 Stefan Richter wrote:
Quoted 5 lines · On Nov 11 Stefan Richter wrote:
Quoted 2 lines · On Nov 10 David Ramos wrote:

This patch fixes an uninitialized memory use/leak bug discovered by our UC-KLEE tool in the 3.16.3 kernel.

There is uninitialized memory use, but no leak.

Actually there could be leaks too. If later fw_cdev_event_ are generated, they will contain the __u64 closure field that many of the ioctl argument structures contain.

Quoted 20 lines

[...]

Quoted 4 lines

If a user carefully crafts an ioctl command such that _IOC_DIR(cmd) == 0, 'buffer' is left uninitialized. Each of the ioctl_handlers then accesses the pre-existing stack values, which will cause unpredictable behavior.

Not all (but indeed almost all) ioctl handlers will use uninitialized memory in that case. The damage should be marginal though, because all of these handlers must implement sufficient arguments checking anyway.

Quoted 1 line

This patch checks for an invalid cmd and rejects it with -ENOTTY.

This is not exactly what your patch does. Rather...

Quoted 6 lines
drivers/firewire/core-cdev.c +0 -0
@@ -1632,7 +1632,8 @@ static int dispatch_ioctl(struct client *client,
 	if (fw_device_is_shutdown(client->device))
 		return -ENODEV;
Quoted 11 lines

drivers/firewire/core-cdev.c static int dispatch_ioctl(struct client *client, · line 1639

Quoted 6 lines
drivers/firewire/core-cdev.c +2 -1
-	if (_IOC_TYPE(cmd) != '#' ||
+	if (_IOC_DIR(cmd) == _IOC_NONE ||
+	    _IOC_TYPE(cmd) != '#' ||
 	    _IOC_NR(cmd) >= ARRAY_SIZE(ioctl_handlers) ||
 	    _IOC_SIZE(cmd) > sizeof(buffer))
 		return -ENOTTY;

...it quits *all* _IOC_NONE ioctls with -ENOTTY, not just invalid ones. If you have a look at the ABI definition in include/uapi/linux/firewire-cdev.h, you will notice that this approach is insufficient.

I suppose we should add checks
1. for _IOC_DIR matching the expected direction depending on _IOC_NR (or at least for presence of _IOC_WRITE when expected), and
2. for _IOC_SIZE being at least as big as the minimum unaligned argument size depending on _IOC_NR (at least in case of _IOC_DIR(cmd) & _IOC_WRITE).

Quoted 4 lines

BTW, I don't think it is worthwhile to check for upper bounds of _IOC_SIZE besides the present check against sizeof(buffer), since the precise size of arguments depends on structure alignment of the user code and on ABI version.

I will follow up with two alternative patches for discussion; either one should fix the issue but they differ in complexity and runtime cost:
[PATCH RFC v1a] firewire: cdev: prevent kernel stack leaking into ioctl arguments [PATCH RFC v1b] firewire: cdev: prevent kernel stack leaking into ioctl arguments
If I receive no feedback to the contrary, I will probably submit patch v1b to upstream, which is the simpler one of the two.

Signature · 10 lines
Stefan Richter
-=====-====- =-== -=-==
http://arcgraph.de/sr/

------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
author
#4 lore raw

RFC PATCH 1/1 firewire: cdev: prevent kernel stack leaking into ioctl arguments

Found by the UC-KLEE tool: A user could supply less input to firewire-cdev ioctls than write- or write/read-type ioctl handlers expect. The handlers used data from uninitialized kernel stack then.

This could partially leak back to the user if the kernel subsequently generated fw_cdev_event_'s (to be read from the firewire-cdev fd) which notably would contain the _u64 closure field which many of the ioctl argument structures contain.

The fact that the handlers would act on random garbage input is a lesser issue since all handlers must check their input anyway.

Remarks:
- There was never any leak from kernel stack to the ioctl output buffer itself. IOW, it was not possible to read kernel stack by a read-type or write/read-type ioctl alone; the leak could at most happen in combination with read()ing subsequent event data.
- The affected character device file interface is specified in include/uapi/linux/firewire-cdev.h. An overview is given in Documentation/ABI/stable/firewire-cdev.

This fix uses a lookup table to verify that all ioctl input fields are indeed written by the user. Else the ioctl fails with -EINVAL.

Reported-by David Ramos <[email protected]>
Cc <[email protected]>
Signed-off-by Stefan Richter <[email protected]>
---
 drivers/firewire/core-cdev.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
drivers/firewire/core-cdev.c +33 -0
@@ -1609,48 +1609,81 @@ static int (* const ioctl_handlers[])(st
 	[0x0a] = ioctl_start_iso,
 	[0x0b] = ioctl_stop_iso,
 	[0x0c] = ioctl_get_cycle_timer,
 	[0x0d] = ioctl_allocate_iso_resource,
 	[0x0e] = ioctl_deallocate_iso_resource,
 	[0x0f] = ioctl_allocate_iso_resource_once,
 	[0x10] = ioctl_deallocate_iso_resource_once,
 	[0x11] = ioctl_get_speed,
 	[0x12] = ioctl_send_broadcast_request,
 	[0x13] = ioctl_send_stream_packet,
 	[0x14] = ioctl_get_cycle_timer2,
 	[0x15] = ioctl_send_phy_packet,
 	[0x16] = ioctl_receive_phy_packets,
 	[0x17] = ioctl_set_iso_channels,
 	[0x18] = ioctl_flush_iso,
 };
 
+static const char minimum_user_input[] = {
+	[0x00] = 32,	/* _IOWR fw_cdev_get_info			*/
+	[0x01] = 36,	/*  _IOW fw_cdev_send_request			*/
+	[0x02] = 20,	/* _IOWR fw_cdev_allocate			*/
+	[0x03] =  4,	/*  _IOW fw_cdev_deallocate			*/
+	[0x04] = 20,	/*  _IOW fw_cdev_send_response			*/
+	[0x05] =  4,	/*  _IOW fw_cdev_initiate_bus_reset		*/
+	[0x06] = 20,	/* _IOWR fw_cdev_add_descriptor			*/
+	[0x07] =  4,	/*  _IOW fw_cdev_remove_descriptor		*/
+	[0x08] = 24,	/* _IOWR fw_cdev_create_iso_context		*/
+	[0x09] = 24,	/* _IOWR fw_cdev_queue_iso			*/
+	[0x0a] = 16,	/*  _IOW fw_cdev_start_iso			*/
+	[0x0b] =  4,	/*  _IOW fw_cdev_stop_iso			*/
+	[0x0c] =  0,	/*  _IOR fw_cdev_get_cycle_timer		*/
+	[0x0d] = 20,	/* _IOWR fw_cdev_allocate_iso_resource		*/
+	[0x0e] =  4,	/*  _IOW fw_cdev_deallocate			*/
+	[0x0f] = 20,	/*  _IOW fw_cdev_allocate_iso_resource		*/
+	[0x10] = 20,	/*  _IOW fw_cdev_allocate_iso_resource		*/
+	[0x11] =  0,	/*   _IO					*/
+	[0x12] = 36,	/*  _IOW struct fw_cdev_send_request		*/
+	[0x13] = 40,	/*  _IOW struct fw_cdev_send_stream_packet	*/
+	[0x14] = 16,	/* _IOWR fw_cdev_get_cycle_timer2		*/
+	[0x15] = 20,	/* _IOWR fw_cdev_send_phy_packet		*/
+	[0x16] =  8,	/*  _IOW fw_cdev_receive_phy_packets		*/
+	[0x17] = 12,	/*  _IOW fw_cdev_set_iso_channels		*/
+	[0x18] =  4,	/*  _IOW struct fw_cdev_flush_iso		*/
+};
+
 static int dispatch_ioctl(struct client *client,
 			  unsigned int cmd, void __user *arg)
 {
 	union ioctl_arg buffer;
 	int ret;
 
 	if (fw_device_is_shutdown(client->device))
 		return -ENODEV;
 
 	if (_IOC_TYPE(cmd) != '#' ||
 	    _IOC_NR(cmd) >= ARRAY_SIZE(ioctl_handlers) ||
 	    _IOC_SIZE(cmd) > sizeof(buffer))
 		return -ENOTTY;
 
+	if (minimum_user_input[_IOC_NR(cmd)])
+		if (!(_IOC_DIR(cmd) & _IOC_WRITE) ||
+		    _IOC_SIZE(cmd) < minimum_user_input[_IOC_NR(cmd)])
+			return -EINVAL;
+
 	if (_IOC_DIR(cmd) == _IOC_READ)
 		memset(&buffer, 0, _IOC_SIZE(cmd));
 
 	if (_IOC_DIR(cmd) & _IOC_WRITE)
 		if (copy_from_user(&buffer, arg, _IOC_SIZE(cmd)))
 			return -EFAULT;
 
 	ret = ioctl_handlers[_IOC_NR(cmd)](client, &buffer);
 	if (ret < 0)
 		return ret;
 
 	if (_IOC_DIR(cmd) & _IOC_READ)
 		if (copy_to_user(arg, &buffer, _IOC_SIZE(cmd)))
 			return -EFAULT;
 
 	return ret;
 }
Signature · 10 lines
Stefan Richter
-=====-====- =-== -=-==
http://arcgraph.de/sr/

------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
author
#5 lore raw

RFC PATCH 1/1 firewire: cdev: prevent kernel stack leaking into ioctl arguments

In reply to Stefan Richter

Found by the UC-KLEE tool: A user could supply less input to firewire-cdev ioctls than write- or write/read-type ioctl handlers expect. The handlers used data from uninitialized kernel stack then.

This could partially leak back to the user if the kernel subsequently generated fw_cdev_event_'s (to be read from the firewire-cdev fd) which notably would contain the _u64 closure field which many of the ioctl argument structures contain.

The fact that the handlers would act on random garbage input is a lesser issue since all handlers must check their input anyway.

Remarks:
- There was never any leak from kernel stack to the ioctl output buffer itself. IOW, it was not possible to read kernel stack by a read-type or write/read-type ioctl alone; the leak could at most happen in combination with read()ing subsequent event data.
- The affected character device file interface is specified in include/uapi/linux/firewire-cdev.h. An overview is given in Documentation/ABI/stable/firewire-cdev.

This fix simply always null-initializes the entire ioctl argument buffer regardless of the actual length of expected user input. That is, a runtime overhead of memset(..., 40) is added to each firewirew-cdev ioctl() call.

Reported-by David Ramos <[email protected]>
Cc <[email protected]>
Signed-off-by Stefan Richter <[email protected]>
---
 drivers/firewire/core-cdev.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/firewire/core-cdev.c +1 -2
@@ -1625,32 +1625,31 @@ static int (* const ioctl_handlers[])(st
 
 static int dispatch_ioctl(struct client *client,
 			  unsigned int cmd, void __user *arg)
 {
 	union ioctl_arg buffer;
 	int ret;
 
 	if (fw_device_is_shutdown(client->device))
 		return -ENODEV;
 
 	if (_IOC_TYPE(cmd) != '#' ||
 	    _IOC_NR(cmd) >= ARRAY_SIZE(ioctl_handlers) ||
 	    _IOC_SIZE(cmd) > sizeof(buffer))
 		return -ENOTTY;
 
-	if (_IOC_DIR(cmd) == _IOC_READ)
-		memset(&buffer, 0, _IOC_SIZE(cmd));
+	memset(&buffer, 0, sizeof(buffer));
 
 	if (_IOC_DIR(cmd) & _IOC_WRITE)
 		if (copy_from_user(&buffer, arg, _IOC_SIZE(cmd)))
 			return -EFAULT;
 
 	ret = ioctl_handlers[_IOC_NR(cmd)](client, &buffer);
 	if (ret < 0)
 		return ret;
 
 	if (_IOC_DIR(cmd) & _IOC_READ)
 		if (copy_to_user(arg, &buffer, _IOC_SIZE(cmd)))
 			return -EFAULT;
 
 	return ret;
 }
Signature · 10 lines
Stefan Richter
-=====-====- =-== -=-==
http://arcgraph.de/sr/

------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
#6 lore raw

Re: [PATCH RFC v1b] firewire: cdev: prevent kernel stack leaking into ioctl arguments

Quoted 4 lines · Stefan Richter

This fix simply always null-initializes the entire ioctl argument buffer regardless of the actual length of expected user input. That is, a runtime overhead of memset(..., 40) is added to each firewirew-cdev ioctl() call.

This part of the stack is most likely to be already in the cache.

Regards,
Clemens