* Re: [PATCH] firewire: cdev: check write quadlet request length to avoid buffer overflow [not found] <4C29C1CA.1050705@ladisch.de> @ 2010-07-07 8:46 ` Stefan Richter 2010-07-07 9:50 ` [PATCH v2] " Stefan Richter 0 siblings, 1 reply; 6+ messages in thread From: Stefan Richter @ 2010-07-07 8:46 UTC (permalink / raw) To: Clemens Ladisch; +Cc: linux1394-devel, linux-kernel On 29 Jun, Clemens Ladisch wrote at linux1394-devel: > Check that the data length of a write quadlet request actually is large > enough for a quadlet. Otherwise, fw_fill_request could access the four > bytes after the end of the outbound_transaction_event structure. > > Signed-off-by: Clemens Ladisch <clemens@ladisch.de> > --- > drivers/firewire/core-cdev.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > --- a/drivers/firewire/core-cdev.c > +++ b/drivers/firewire/core-cdev.c > @@ -593,6 +593,9 @@ static int ioctl_send_request(struct cli > { > switch (arg->send_request.tcode) { > case TCODE_WRITE_QUADLET_REQUEST: > + if (arg->send_request.length < 4) > + return -EINVAL; > + break; > case TCODE_WRITE_BLOCK_REQUEST: > case TCODE_READ_QUADLET_REQUEST: > case TCODE_READ_BLOCK_REQUEST: > @@ -1293,6 +1296,9 @@ static int ioctl_send_broadcast_request( > > switch (a->tcode) { > case TCODE_WRITE_QUADLET_REQUEST: > + if (a->length < 4) > + return -EINVAL; > + break; > case TCODE_WRITE_BLOCK_REQUEST: > break; > default: The fix is correct but apparently not urgent. Here is a listing of struct outbound_transaction_event's definition, with member sizes in bytes prepended. First column is on a 32bit architecture, second column on a 64bit architecture. Type definitions are as per 2.6.34. struct outbound_transaction_event { struct event { 16 32 struct { void *data; size_t size; } v[2]; struct list_head { 8 16 struct list_head *next, *prev; } link; } event; 4 8 struct client *client; struct outbound_transaction_resource { struct client_resource { 4 8 client_resource_release_fn_t release; 4 4 int handle; } resource; struct fw_transaction { 4 4 int node_id; 4 4 int tlabel; 4 4 int timestamp; struct list_head { 8 16 struct list_head *next, *prev; } link; 4 8 struct fw_card *card; struct timer_list { struct list_head { 8 16 struct list_head *next, *prev; } entry; 4 8 unsigned long expires; 4 8 void (*function)(unsigned long); 4 8 unsigned long data; 4 8 struct tvec_base *base; #ifdef CONFIG_TIMER_STATS 4* 8* void *start_site; 16* 16* char start_comm[16]; 4* 4* int start_pid; #endif #ifdef CONFIG_LOCKDEP struct lockdep_map { 4* 12*° struct lock_class_key *key; 4* 8* struct lock_class *class_cache; 4* 8* const char *name; #ifdef CONFIG_LOCK_STAT 4* 4* int cpu; 4* 8* unsigned long ip; #endif } lockdep_map; #endif } split_timeout_timer; struct fw_packet { 4 4 int speed; 4 4 int generation; 16 16 u32 header[4]; 4 8 size_t header_length; 4 8 void *payload; 4 8 size_t payload_length; 4^ 8 dma_addr_t payload_bus; 4 4 bool payload_mapped; 4 4 u32 timestamp; 4 8 fw_packet_callback_t callback; 4 4 int ack; struct list_head { 8 20° struct list_head *next, *prev; } link; 4 8 void *driver_data; } packet; 4 8 fw_transaction_callback_t callback; 4 8 void *callback_data; } transaction; } r; struct fw_cdev_event_response { 8 8 __u64 closure; 4 4 __u32 type; 4 4 __u32 rcode; 4 4 __u32 length; __u32 data[0]; } response; }; *) =0 if respective debug options are off °) -4 if the compiler aligns 64bit types at 32bit boundaries ^) =8 if CONFIG_HIGHMEM64G The total size is therefore 180 bytes on 32bit without debug options and HIGHMEM64G off 228 bytes on 32bit with TIMER_STATS, LOCKDEP, LOCK_STAT, and HIGHMEM64G 292 bytes on 64bit without debug options 360 bytes on 64bit with TIMER_STATS, LOCKDEP, and LOCK_STAT Therefore the slab allocator gives us 256 bytes on 32bit and 512 bytes on 64bit, right? Or more if the user-requested size of response.data[] raises the total size above that. (struct outbound_transaction_event instances are always kmalloc'ed, never stack-allocated or kmem_cache-allocated.) Which in turn means that an access past the requested size still lands in allocated (though uninitialized) memory. I.e. nothing bad happens except that random bytes are included into the quadlet write request packet payload. (I only ask because I wonder about how to submit this patch or a variant of it. Me thinks the post 2.6.35 merge is OK.) -- Stefan Richter -=====-==-=- -=== --=== http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] firewire: cdev: check write quadlet request length to avoid buffer overflow 2010-07-07 8:46 ` [PATCH] firewire: cdev: check write quadlet request length to avoid buffer overflow Stefan Richter @ 2010-07-07 9:50 ` Stefan Richter 2010-07-07 10:01 ` [PATCH v3] " Stefan Richter 0 siblings, 1 reply; 6+ messages in thread From: Stefan Richter @ 2010-07-07 9:50 UTC (permalink / raw) To: Clemens Ladisch; +Cc: linux1394-devel, linux-kernel Check that the data length of a write quadlet request actually is large enough for a quadlet. Otherwise, fw_fill_request could access the four bytes after the end of the outbound_transaction_event structure. Reported-by: Clemens Ladisch <clemens@ladisch.de> Since struct outbound_transaction_event *e is slab-allocated, such an access may hit unallocated memory only if sizeof(*e) == 256 or 512 or any other power of 2 above 2**2. In kernel 2.6.34, sizeof(*e) is > 128 and < 256 on 32bit architectures, and > 256 and < 512 on 64bit architectures. Thus the only problem is that a bogus write quadlet request with user-specified length of < 3 will put 1...4 random bytes into the packet payload; but this is the users's problem then, not the kernel's. Hence the corner case handling can be optimized by a !(sizeof(*e) & 255) check as a weaker form of a sizeof(*e) == 256 or 512 check. This is a constant expression and will cause the whole check to be omitted by the compiler's dead code elimination. Of course this relies on certain behavior of the slab allocator. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- drivers/firewire/core-cdev.c | 5 +++++ 1 file changed, 5 insertions(+) Index: b/drivers/firewire/core-cdev.c =================================================================== --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -564,6 +564,11 @@ static int init_request(struct client *c (request->length > 4096 || request->length > 512 << speed)) return -EIO; + /* Corner case: Access past the end of *e in fw_fill_request() */ + if (request->tcode == TCODE_WRITE_QUADLET_REQUEST && + request->length < 4 && (sizeof(*e) & 255) == 0) + return -EINVAL; + e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL); if (e == NULL) return -ENOMEM; -- Stefan Richter -=====-==-=- -=== --=== http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] firewire: cdev: check write quadlet request length to avoid buffer overflow 2010-07-07 9:50 ` [PATCH v2] " Stefan Richter @ 2010-07-07 10:01 ` Stefan Richter 2010-07-07 11:55 ` Clemens Ladisch 0 siblings, 1 reply; 6+ messages in thread From: Stefan Richter @ 2010-07-07 10:01 UTC (permalink / raw) To: Clemens Ladisch; +Cc: linux1394-devel, linux-kernel Check that the data length of a write quadlet request actually is large enough for a quadlet. Otherwise, fw_fill_request could access the four bytes after the end of the outbound_transaction_event structure. Reported-by: Clemens Ladisch <clemens@ladisch.de> Since struct outbound_transaction_event *e is slab-allocated, such an access may hit unallocated memory only if sizeof(*e) == 256 or 512 or any other power of 2 above 2**2. In kernel 2.6.34, sizeof(*e) is > 128 and < 256 on 32bit architectures, and > 256 and < 512 on 64bit architectures. Thus the only problem is that a bogus write quadlet request with user-specified length of < 3 will put 1...4 random bytes into the packet payload. But this is the user's problem then, not the kernel's. Hence the corner case handling can be optimized by a size is_power_of_2 check. This is a constant expression and will cause the whole check to be omitted by the compiler's dead code elimination. Of course this relies on certain behavior of the slab allocator. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- drivers/firewire/core-cdev.c | 6 ++++++ 1 file changed, 6 insertions(+) Index: b/drivers/firewire/core-cdev.c =================================================================== --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -29,6 +29,7 @@ #include <linux/jiffies.h> #include <linux/kernel.h> #include <linux/kref.h> +#include <linux/log2.h> #include <linux/mm.h> #include <linux/module.h> #include <linux/mutex.h> @@ -564,6 +565,11 @@ static int init_request(struct client *c (request->length > 4096 || request->length > 512 << speed)) return -EIO; + /* Corner case: Access past the end of *e in fw_fill_request() */ + if (request->tcode == TCODE_WRITE_QUADLET_REQUEST && + request->length < 4 && is_power_of_2(sizeof(*e))) + return -EINVAL; + e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL); if (e == NULL) return -ENOMEM; -- Stefan Richter -=====-==-=- -=== --=== http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] firewire: cdev: check write quadlet request length to avoid buffer overflow 2010-07-07 10:01 ` [PATCH v3] " Stefan Richter @ 2010-07-07 11:55 ` Clemens Ladisch 2010-07-07 12:20 ` Stefan Richter 0 siblings, 1 reply; 6+ messages in thread From: Clemens Ladisch @ 2010-07-07 11:55 UTC (permalink / raw) To: Stefan Richter; +Cc: linux1394-devel, linux-kernel Stefan Richter wrote: > [...] Thus the only problem is that a bogus write quadlet > request with user-specified length of < 3 will put 1...4 random bytes > into the packet payload. But this is the user's problem then, not the > kernel's. But not being initialized, these are the kernel's bytes that get disclosed. Regards, Clemens ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] firewire: cdev: check write quadlet request length to avoid buffer overflow 2010-07-07 11:55 ` Clemens Ladisch @ 2010-07-07 12:20 ` Stefan Richter 2010-07-07 12:37 ` [PATCH v4] " Stefan Richter 0 siblings, 1 reply; 6+ messages in thread From: Stefan Richter @ 2010-07-07 12:20 UTC (permalink / raw) To: Clemens Ladisch; +Cc: linux1394-devel, linux-kernel On 7 Jul, Clemens Ladisch wrote: > Stefan Richter wrote: >> [...] Thus the only problem is that a bogus write quadlet >> request with user-specified length of < 3 will put 1...4 random bytes >> into the packet payload. But this is the user's problem then, not the >> kernel's. > > But not being initialized, these are the kernel's bytes that get > disclosed. Yes. In which way can this be exploited though? For all practical purposes, the signal-to-noise ratio of these 1...4 bytes seems to be 0. -- Stefan Richter -=====-==-=- -=== --=== http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4] firewire: cdev: check write quadlet request length to avoid buffer overflow 2010-07-07 12:20 ` Stefan Richter @ 2010-07-07 12:37 ` Stefan Richter 0 siblings, 0 replies; 6+ messages in thread From: Stefan Richter @ 2010-07-07 12:37 UTC (permalink / raw) To: Clemens Ladisch; +Cc: linux1394-devel, linux-kernel From: Clemens Ladisch <clemens@ladisch.de> Check that the data length of a write quadlet request actually is large enough for a quadlet. Otherwise, fw_fill_request could access the four bytes after the end of the outbound_transaction_event structure. Signed-off-by: Clemens Ladisch <clemens@ladisch.de> Modification of Clemens' change: Consolidate the check in init_request() which is used by the affected ioctl_send_request() and ioctl_send_broadcast_request() and the unaffected ioctl_send_stream_packet(), to save a few lines of code. Note, since struct outbound_transaction_event *e is slab-allocated, such an out-of-bounds access won't hit unallocated memory but may result in a (virtually impossible to exploit) information disclosure. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> --- drivers/firewire/core-cdev.c | 4 ++++ 1 file changed, 4 insertions(+) Index: b/drivers/firewire/core-cdev.c =================================================================== --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -564,6 +564,10 @@ static int init_request(struct client *c (request->length > 4096 || request->length > 512 << speed)) return -EIO; + if (request->tcode == TCODE_WRITE_QUADLET_REQUEST && + request->length < 4) + return -EINVAL; + e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL); if (e == NULL) return -ENOMEM; -- Stefan Richter -=====-==-=- -=== --=== http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-07-07 12:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4C29C1CA.1050705@ladisch.de>
2010-07-07 8:46 ` [PATCH] firewire: cdev: check write quadlet request length to avoid buffer overflow Stefan Richter
2010-07-07 9:50 ` [PATCH v2] " Stefan Richter
2010-07-07 10:01 ` [PATCH v3] " Stefan Richter
2010-07-07 11:55 ` Clemens Ladisch
2010-07-07 12:20 ` Stefan Richter
2010-07-07 12:37 ` [PATCH v4] " Stefan Richter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.