[CHECKER] 4 security holes in 2.4.4-ac8

[CHECKER] 4 security holes in 2.4.4-ac8

[Dawson Engler]

Hi All,

Enclosed are four bugs where 2.4.4-ac8 kernel code appears to directly
read/write user space pointers.  The latter three were found after
forming equivalence classes by:
	(1) recording all routines assigned to the same function pointer
	    field in a structure
	(2) if any member of the equiv class treated a parameter as a
	    user-space pointer, checking that they all do.

Let me know if any of these are wrong.  The first one seems pretty bad.

Dawson
-------------------------------------------------------------------------------
[BUG] raddr seems to be a user pointer, but is written at the end of
      the system call.

ipc/shm.c: ERROR: system call 'sys_shmat' derefs non-tainted param= 3

asmlinkage long sys_shmat (int shmid, char *shmaddr, int shmflg, ulong *raddr)
{
        struct shmid_kernel *shp;


	...
        *raddr = (unsigned long) user_addr;
        err = 0;
        if (IS_ERR(user_addr))
                err = PTR_ERR(user_addr);
        return err;

-----------------------------------------------------------------------------
[BUG] ./drivers/usb/bluetooth.c: dereference of 'buf' at the beginning of
      the switch, and then does a copyin.

	ERROR: equivalence class <struct tty_driver:write> contains 
	2 functions that taint parameter <2>, and 1
	functions that dereference it raw.

	Tainting functions
		[ acm_tty_write:acm.c ]
		[ serial_write:usbserial.c ]
	Dereferencing functions
		[ bluetooth_write:bluetooth.c ]


        switch (*buf) {
                /* First byte indicates the type of packet */
                case CMD_PKT:
                        /* dbg(__FUNCTION__ "- Send cmd_pkt len:%d", count);*/

                        if (in_interrupt()){
                                printk("cmd_pkt from interrupt!\n");
                                return count;
                        }

                        new_buffer = kmalloc (count-1, GFP_KERNEL);

                        if (!new_buffer) {
                                err (__FUNCTION__ "- out of memory.");
                                return -ENOMEM;
                        }

                        if (from_user)
                                copy_from_user (new_buffer, buf+1, count-1);
                        else
                                memcpy (new_buffer, buf+1, count-1);

-------------------------------------------------------
In the equivalence class for file_operations:write: 55 functions treat
their second parameter as tainted, but two functions use it raw.

[BUG]
/* drivers/char/sbc60xxwdt.c: buf is tainted */
static ssize_t fop_write(struct file * file, const char * buf, size_t count, loff_t * ppos)
{       
        /* We can't seek */
        if(ppos != &file->f_pos)
                return -ESPIPE;
        
        /* See if we got the magic character */
        if(count) 
        {
                size_t ofs;
                
                /* note: just in case someone wrote the magic character
                 * five months ago... */
                wdt_expect_close = 0;
                
                /* now scan */
                for(ofs = 0; ofs != count; ofs++)
                        if(buf[ofs] == 'V')
                                wdt_expect_close = 1;
 
                /* Well, anyhow someone wrote to us, we should return that favour */
                next_heartbeat = jiffies + WDT_HEARTBEAT;
        }
        return 0;
}               


[BUG]
/* drivers/usb/mdc800.c: buf is tainted */
static ssize_t mdc800_device_write (struct file *file, const char *buf, size_t len, loff_t *pos)
{               
        int i=0;
 
        spin_lock (&mdc800->io_lock);
        if (mdc800->state != READY)
        {       
                spin_unlock (&mdc800->io_lock);
                return -EBUSY;
        }       
        if (!mdc800->open )
        {       
                spin_unlock (&mdc800->io_lock);
                return -EBUSY;
        }       
 
        while (i<len)
        {       
                if (signal_pending (current))
                { 
                        spin_unlock (&mdc800->io_lock);
                        return -EINTR;
                }
        
                /* check for command start */
                if (buf [i] == (char) 0x55)
                { 
                        mdc800->in_count=0;



Here's the equiv classes:

	Tainting functions
		[ ac_write:applicom.c ]
		[ affs_file_write:file.c ]
		[ block_write:ksyms.c ]
		[ camera_write:dc2xx.c ]
		[ cap_info_write:file_cap.c ]
		[ capi_write:capi.c ]
		[ capinc_raw_write:capi.c ]
		[ coda_file_write:file.c ]
		[ coda_psdev_write:psdev.c ]
		[ cs4281_midi_write:cs4281m.c ]
		[ cs4281_write:cs4281m.c ]
		[ dev_irnet_write:irnet_ppp.h ]
		[ dev_write:raw1394.c ]
		[ ds_write:ds.c ]
		[ dtlk_write:dtlk.c ]
		[ emu10k1_audio_write:audio.c ]
		[ emu10k1_midi_write:midi.c ]
		[ generic_file_write:ksyms.c ]
		[ hdr_write:file_hdr.c ]
		[ hfs_file_write:file.c ]
		[ hpfs_file_write:inode.c ]
		[ i2cdev_write:i2c-dev.c ]
		[ idetape_chrdev_write:ide-tape.c ]
		[ isapnp_info_entry_write:isapnp_proc.c ]
		[ isdn_write:isdn_common.c ]
		[ ixj_enhanced_write:ixj.c ]
		[ lp_write:lp.c ]
		[ mem_write:pcilynx.c ]
		[ microcode_write:microcode.c ]
		[ mtd_write:mtdchar.c ]
		[ mtrr_write:mtrr.c ]
		[ ncp_file_write:file.c ]
		[ nfs_file_write:file.c ]
		[ nvram_write:nvram.c ]
		[ osst_write:osst.c ]
		[ pg_write:pg.c ]
		[ pipe_write:pipe.c ]
		[ ppp_write:ppp_generic.c ]
		[ proc_bus_pci_write:proc.c ]
		[ proc_mpc_write:mpoa_proc.c ]
		[ qic02_tape_write:tpqic02.c ]
		[ sg_write:sg.c ]
		[ shmem_file_write:shmem.c ]
		[ smb_file_write:file.c ]
		[ st_write:st.c ]
		[ tun_chr_write:tun.c ]
		[ usb_audio_write:audio.c ]
		[ vcs_write:vc_screen.c ]
		[ write_kmem:mem.c ]
		[ write_mem:mem.c ]
		[ write_port:mem.c ]
		[ write_profile:proc_misc.c ]
		[ write_rio:rio500.c ]
		[ write_scanner:scanner.c ]
		[ zft_write:zftape-init.c ]
	Dereferencing functions
		[ fop_write:sbc60xxwdt.c ]
		[ mdc800_device_write:mdc800.c ]

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[David S. Miller]

Dawson Engler writes:
 > -------------------------------------------------------------------------------
 > [BUG] raddr seems to be a user pointer, but is written at the end of
 >       the system call.
 > 
 > ipc/shm.c: ERROR: system call 'sys_shmat' derefs non-tainted param= 3
 > 
 > asmlinkage long sys_shmat (int shmid, char *shmaddr, int shmflg, ulong *raddr)
 > {
 >         struct shmid_kernel *shp;
 > 
 > 
 > 	...
 >         *raddr = (unsigned long) user_addr;
 >         err = 0;
 >         if (IS_ERR(user_addr))
 >                 err = PTR_ERR(user_addr);
 >         return err;

Believe it or not, this one is OK :-)

All callers pass in a pointer to a local stack kernel variable
in raddr.

Later,
David S. Miller
davem@redhat.com

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[Dawson Engler]

>  > [BUG] raddr seems to be a user pointer, but is written at the end of
>  >       the system call.
>  > 
>  > ipc/shm.c: ERROR: system call 'sys_shmat' derefs non-tainted param= 3
>  > 
>  > asmlinkage long sys_shmat (int shmid, char *shmaddr, int shmflg, ulong *raddr)
>  > {
>  >         struct shmid_kernel *shp;
>  > 
>  > 
>  > 	...
>  >         *raddr = (unsigned long) user_addr;
>  >         err = 0;
>  >         if (IS_ERR(user_addr))
>  >                 err = PTR_ERR(user_addr);
>  >         return err;
> 
> Believe it or not, this one is OK :-)
> 
> All callers pass in a pointer to a local stack kernel variable
> in raddr.

Ah.  I assumed that "sys_*" meant that all pointers were from user space ---
is this generally not the case?  (Also, are there other functions called 
directly from user space that don't have the sys_* prefix?)

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[David S. Miller]

Dawson Engler writes:
 > Ah.  I assumed that "sys_*" meant that all pointers were from user space ---
 > is this generally not the case?

This shared memory syscall is just a weird exception.

 > (Also, are there other functions called 
 > directly from user space that don't have the sys_* prefix?)

Almost certainly, arch/i386/mm/fault.c:do_page_fault is one of
many examples.

Later,
David S. Miller
davem@redhat.com

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[Dawson Engler]

>  > (Also, are there other functions called 
>  > directly from user space that don't have the sys_* prefix?)
> 
> Almost certainly, arch/i386/mm/fault.c:do_page_fault is one of
> many examples.

Is there any way to automatically find these?  E.g., is any routine
with "asmlinkage" callable from user space?

Dawson

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[David S. Miller]

Dawson Engler writes:
 > Is there any way to automatically find these?  E.g., is any routine
 > with "asmlinkage" callable from user space?

This is only universally done in generic and x86 specific code,
other ports tend to forget asmlinkage simply because most ports
don't need it.

Later,
David S. Miller
davem@redhat.com

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[Jeff Garzik]

Dawson Engler wrote:
> 
> >  > (Also, are there other functions called
> >  > directly from user space that don't have the sys_* prefix?)
> >
> > Almost certainly, arch/i386/mm/fault.c:do_page_fault is one of
> > many examples.
> 
> Is there any way to automatically find these?  E.g., is any routine
> with "asmlinkage" callable from user space?

Checking the syscall table in each port is the only authoritative way
AFAIK.

And, if we start doing "magic page" type entry points, or if special
traps exist on other arches, then those would have to be
special-cased...

-- 
Jeff Garzik      | Disbelief, that's why you fail.
Building 1024    |
MandrakeSoft     |

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[Ingo Molnar]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4507 bytes --]


On Tue, 29 May 2001, Dawson Engler wrote:

> > Believe it or not, this one is OK :-)
> >
> > All callers pass in a pointer to a local stack kernel variable
> > in raddr.
>
> Ah.  I assumed that "sys_*" meant that all pointers were from user
> space --- is this generally not the case?  (Also, are there other
> functions called directly from user space that don't have the sys_*
> prefix?)

to automate this for the Stanford checker i've attached the
'getuserfunctions' script that correctly extracts these function names
from the 2.4.5 x86 entry.S file.

unfortunately the validation of the script will always be manual work,
although for the lifetime of the 2.4 kernel the actual format of the
entry.S file is not going to change. To make this automatic, i've added a
md5sum to the script itself, if entry.S changes then someone has to review
the changes manually. It's important to watch the md5 checksum, because
new system-calls can be added in 2.4 as well.

a few interesting facts. Functions that are called from entry.S but do not
have the sys_ prefix:

 do_nmi
 do_signal
 do_softirq
 old_mmap
 old_readdir
 old_select
 save_v86_state
 schedule
 schedule_tail
 syscall_trace
 do_divide_error
 do_coprocessor_error
 do_simd_coprocessor_error
 do_debug
 do_int3
 do_overflow
 do_bounds
 do_invalid_op
 do_coprocessor_segment_overrun
 do_double_fault
 do_invalid_TSS
 do_segment_not_present
 do_stack_segment
 do_general_protection
 do_alignment_check
 do_page_fault
 do_machine_check
 do_spurious_interrupt_bug

functions in the kernel source that have the sys_ prefix and use
asmlinkage but are not called from the x86 entry.S file:

 sys_accept
 sys_bind
 sys_connect
 sys_gethostname
 sys_getpeername
 sys_getsockname
 sys_getsockopt
 sys_listen
 sys_msgctl
 sys_msgget
 sys_msgrcv
 sys_msgsnd
 sys_recv
 sys_recvfrom
 sys_recvmsg
 sys_semctl
 sys_semget
 sys_semop
 sys_send
 sys_sendmsg
 sys_sendto
 sys_setsockopt
 sys_shmat
 sys_shmctl
 sys_shmdt
 sys_shmget
 sys_shutdown
 sys_socket
 sys_socketpair
 sys_utimes

the list is pretty big. There are 33 functions that are called from
entry.S but do not have the sys_ prefix or do not have the asmlinkage
declaration.

NOTE: there are other entry points into the kernel's 'protection domain'
as well, and not all of them are through function interfaces. Some of
these interfaces pass untrusted pointers and/or untrusted parameters
directly, but most of them pass a pointer to a CPU registers structure
which is stored on the kernel stack (thus the pointer can be trusted), but
the contents of the registers structure are untrusted and must not be used
unchecked.

1) IRQ handling, trap handling, exception handling entry points. I've
atttached the 'getentrypoints' script that extracts these addresses from
the i386 tree:

 divide_error
 debug
 int3
 overflow
 bounds
 invalid_op
 device_not_available
 double_fault
 coprocessor_segment_overrun
 invalid_TSS
 segment_not_present
 stack_segment
 general_protection
 spurious_interrupt_bug
 coprocessor_error
 alignment_check
 machine_check
 simd_coprocessor_error
 system_call
 lcall7
 lcall27

all of these functions get parameters passed that are untrusted.

2) bootup parameter passing.

there is a function entry point, start_kernel, but there is also lots of
implicit parameter passing, values filled out by the boot code, and
parameters stored in hardware devices (eg. PCI settings and more). These
all are theoretical protection domain entry points, but impossible to
check automatically - the validity of current system state will have to be
checked manually. (and in most cases it can be trusted - but not all
cases.) Some 'unexpected' boot-time entry points: initialize_secondary on
SMP systems for example.

3) manually constructed unsafe entry points which are hard to automate.
include/asm-i386/hw_irq.h's BUILD macros are used in a number of places.
One type of IRQ building uses do_IRQ() as an entry point. The SMP
code builds the following entry points:

 reschedule_interrupt
 invalidate_interrupt
 call_function_interrupt
 apic_timer_interrupt
 error_interrupt
 spurious_interrupt

but most of these pass no parameters, but apic_timer_interrupt does get
untrusted parameters.

4) BIOS exit/entry points, eg in the APM code. Impossible to check, we
have to trust the BIOS's code.


i think this mail should be a more or less complete description of all
entry points into the kernel. (Let me know if i missed any of them, or any
of the scripts misidentifies entry points.)

	Ingo

[-- Attachment #2: Type: TEXT/PLAIN, Size: 160 bytes --]


grep -E 'set_trap_gate|set_system_gate|set_call_gate' arch/i386/*/*.c arch/i386/*/*.h | grep -v 'static void' | cut -d, -f2- | sed 's/&//g' | cut -d\) -f1


[-- Attachment #3: Type: TEXT/PLAIN, Size: 441 bytes --]


if [ "`md5sum arch/i386/kernel/entry.S`" != "0e19b0892f4bd25015f5f1bfe90b441a  arch/i386/kernel/entry.S" ]; then echo "entry.S file's MD5sum changed! Please revalidate the changes and change the md5sum in this script."; exit -1; fi

(grep 'call S' arch/i386/kernel/entry.S  | grep '[()]'; grep '\.long SYMBOL_NAME' arch/i386/kernel/entry.S; grep 'pushl \$ SYMBOL' arch/i386/kernel/entry.S) | cut -d\( -f2 | cut -d\) -f1 | sort | uniq

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

On Tue, May 29, 2001 at 03:00:56PM -0700, Dawson Engler wrote:
> -----------------------------------------------------------------------------
> [BUG] ./drivers/usb/bluetooth.c: dereference of 'buf' at the beginning of
>       the switch, and then does a copyin.

This is a real bug, thanks.
The attached patch, against the latest -ac tree should fix it.

greg k-h

[-- Attachment #2: usb-bluetooth-2-2.4.5.patch --]
[-- Type: text/plain, Size: 7820 bytes --]

diff -Nru a/drivers/usb/bluetooth.c b/drivers/usb/bluetooth.c
--- a/drivers/usb/bluetooth.c	Wed May 30 11:10:08 2001
+++ b/drivers/usb/bluetooth.c	Wed May 30 11:10:08 2001
@@ -1,11 +1,20 @@
 /*
- * bluetooth.c   Version 0.9
+ * bluetooth.c   Version 0.10
  *
  * Copyright (c) 2000, 2001 Greg Kroah-Hartman	<greg@kroah.com>
  * Copyright (c) 2000 Mark Douglas Corner	<mcorner@umich.edu>
  *
  * USB Bluetooth driver, based on the Bluetooth Spec version 1.0B
  * 
+ * (2001/05/28) Version 0.10 gkh
+ *	- Fixed problem with using data from userspace in the bluetooth_write
+ *	  function as found by the CHECKER project.
+ *	- Added a buffer to the write_urb_pool which reduces the number of
+ *	  buffers being created and destroyed for ever write.  Also cleans
+ *	  up the logic a bit.
+ *	- Added a buffer to the control_urb_pool which fixes a memory leak
+ *	  when the device is removed from the system.
+ *
  * (2001/05/28) Version 0.9 gkh
  *	Fixed problem with bluetooth==NULL for bluetooth_read_bulk_callback
  *	which was found by both the CHECKER project and Mikko Rahkonen.
@@ -101,7 +110,7 @@
 /*
  * Version Information
  */
-#define DRIVER_VERSION "v0.9"
+#define DRIVER_VERSION "v0.10"
 #define DRIVER_AUTHOR "Greg Kroah-Hartman, Mark Douglas Corner"
 #define DRIVER_DESC "USB Bluetooth driver"
 
@@ -264,7 +273,7 @@
 }
 
 
-static int bluetooth_ctrl_msg (struct usb_bluetooth *bluetooth, int request, int value, void *buf, int len)
+static int bluetooth_ctrl_msg (struct usb_bluetooth *bluetooth, int request, int value, const unsigned char *buf, int len)
 {
 	struct urb *urb = NULL;
 	devrequest *dr = NULL;
@@ -286,11 +295,23 @@
 		return -ENOMEM;
 	}
 
-	/* free up the last buffer that this urb used */
-	if (urb->transfer_buffer != NULL) {
-		kfree(urb->transfer_buffer);
-		urb->transfer_buffer = NULL;
+	/* keep increasing the urb transfer buffer to fit the size of the message */
+	if (urb->transfer_buffer == NULL) {
+		urb->transfer_buffer = kmalloc (len, GFP_KERNEL);
+		if (urb->transfer_buffer == NULL) {
+			err (__FUNCTION__" - out of memory");
+			return -ENOMEM;
+		}
+	}
+	if (urb->transfer_buffer_length < len) {
+		kfree (urb->transfer_buffer);
+		urb->transfer_buffer = kmalloc (len, GFP_KERNEL);
+		if (urb->transfer_buffer == NULL) {
+			err (__FUNCTION__" - out of memory");
+			return -ENOMEM;
+		}
 	}
+	memcpy (urb->transfer_buffer, buf, len);
 
 	dr->requesttype = BLUETOOTH_CONTROL_REQUEST_TYPE;
 	dr->request = request;
@@ -299,14 +320,14 @@
 	dr->length = cpu_to_le16p(&len);
 	
 	FILL_CONTROL_URB (urb, bluetooth->dev, usb_sndctrlpipe(bluetooth->dev, 0),
-			  (unsigned char*)dr, buf, len, bluetooth_ctrl_callback, bluetooth);
+			  (unsigned char*)dr, urb->transfer_buffer, len, bluetooth_ctrl_callback, bluetooth);
 
 	/* send it down the pipe */
 	status = usb_submit_urb(urb);
 	if (status)
 		dbg(__FUNCTION__ " - usb_submit_urb(control) failed with status = %d", status);
 	
-	return 0;
+	return status;
 }
 
 
@@ -405,12 +426,13 @@
 {
 	struct usb_bluetooth *bluetooth = get_usb_bluetooth ((struct usb_bluetooth *)tty->driver_data, __FUNCTION__);
 	struct urb *urb = NULL;
-	unsigned char *new_buffer;
+	unsigned char *temp_buffer = NULL;
+	const unsigned char *current_buffer;
 	const unsigned char *current_position;
-	int status;
 	int bytes_sent;
 	int buffer_size;
 	int i;
+	int retval = 0;
 
 	if (!bluetooth) {
 		return -ENODEV;
@@ -440,38 +462,39 @@
 	printk ("\n");
 #endif
 
-	switch (*buf) {
+	if (from_user) {
+		temp_buffer = kmalloc (count, GFP_KERNEL);
+		if (temp_buffer == NULL) {
+			err (__FUNCTION__ "- out of memory.");
+			retval = -ENOMEM;
+			goto exit;
+		}
+		copy_from_user (temp_buffer, buf, count);
+		current_buffer = temp_buffer;
+	} else {
+		current_buffer = buf;
+	}
+
+	switch (*current_buffer) {
 		/* First byte indicates the type of packet */
 		case CMD_PKT:
 			/* dbg(__FUNCTION__ "- Send cmd_pkt len:%d", count);*/
 
 			if (in_interrupt()){
 				printk("cmd_pkt from interrupt!\n");
-				return count;
-			}
-
-			new_buffer = kmalloc (count-1, GFP_KERNEL);
-
-			if (!new_buffer) {
-				err (__FUNCTION__ "- out of memory.");
-				return -ENOMEM;
+				retval = count;
+				goto exit;
 			}
 
-			if (from_user)
-				copy_from_user (new_buffer, buf+1, count-1);
-			else
-				memcpy (new_buffer, buf+1, count-1);
-
-			if (bluetooth_ctrl_msg (bluetooth, 0x00, 0x00, new_buffer, count-1) != 0) {
-				kfree (new_buffer);
-				return 0;
+			retval = bluetooth_ctrl_msg (bluetooth, 0x00, 0x00, &current_buffer[1], count-1);
+			if (retval) {
+				goto exit;
 			}
-
-			/* need to free new_buffer somehow... FIXME */
-			return count;
+			retval = count;
+			break;
 
 		case ACL_PKT:
-			current_position = buf;
+			current_position = current_buffer;
 			++current_position;
 			--count;
 			bytes_sent = 0;
@@ -488,37 +511,25 @@
 				}
 				if (urb == NULL) {
 					dbg (__FUNCTION__ " - no free urbs");
-					return bytes_sent;
+					retval = bytes_sent;
+					goto exit;
 				}
 				
-				/* free up the last buffer that this urb used */
-				if (urb->transfer_buffer != NULL) {
-					kfree(urb->transfer_buffer);
-					urb->transfer_buffer = NULL;
-				}
 
 				buffer_size = MIN (count, bluetooth->bulk_out_buffer_size);
-				
-				new_buffer = kmalloc (buffer_size, GFP_KERNEL);
-				if (new_buffer == NULL) {
-					err(__FUNCTION__" no more kernel memory...");
-					return bytes_sent;
-				}
-
-				if (from_user)
-					copy_from_user(new_buffer, current_position, buffer_size);
-				else
-					memcpy (new_buffer, current_position, buffer_size);
+				memcpy (urb->transfer_buffer, current_position, buffer_size);
 
 				/* build up our urb */
 				FILL_BULK_URB (urb, bluetooth->dev, usb_sndbulkpipe(bluetooth->dev, bluetooth->bulk_out_endpointAddress),
-						new_buffer, buffer_size, bluetooth_write_bulk_callback, bluetooth);
+						urb->transfer_buffer, buffer_size, bluetooth_write_bulk_callback, bluetooth);
 				urb->transfer_flags |= USB_QUEUE_BULK;
 
 				/* send it down the pipe */
-				status = usb_submit_urb(urb);
-				if (status)
-					dbg(__FUNCTION__ " - usb_submit_urb(write bulk) failed with status = %d", status);
+				retval = usb_submit_urb(urb);
+				if (retval) {
+					dbg(__FUNCTION__ " - usb_submit_urb(write bulk) failed with error = %d", retval);
+					goto exit;
+				}
 #ifdef BTBUGGYHARDWARE
 				/* A workaround for the stalled data bug */
 				/* May or may not be needed...*/
@@ -531,13 +542,20 @@
 				count -= buffer_size;
 			}
 
-			return bytes_sent + 1;
+			retval = bytes_sent + 1;
+			break;
 		
 		default :
 			dbg(__FUNCTION__" - unsupported (at this time) write type");
+			retval = -EINVAL;
+			break;
 	}
 
-	return 0;
+exit:
+	if (temp_buffer != NULL)
+		kfree (temp_buffer);
+
+	return retval;
 } 
 
 
@@ -1121,7 +1139,11 @@
 			err("No free urbs available");
 			goto probe_error;
 		}
-		urb->transfer_buffer = NULL;
+		urb->transfer_buffer = kmalloc (bluetooth->bulk_out_buffer_size, GFP_KERNEL);
+		if (urb->transfer_buffer == NULL) {
+			err("out of memory");
+			goto probe_error;
+		}
 		bluetooth->write_urb_pool[i] = urb;
 	}
 	
@@ -1163,11 +1185,17 @@
 	if (bluetooth->interrupt_in_buffer)
 		kfree (bluetooth->interrupt_in_buffer);
 	for (i = 0; i < NUM_BULK_URBS; ++i)
-		if (bluetooth->write_urb_pool[i])
+		if (bluetooth->write_urb_pool[i]) {
+			if (bluetooth->write_urb_pool[i]->transfer_buffer)
+				kfree (bluetooth->write_urb_pool[i]->transfer_buffer);
 			usb_free_urb (bluetooth->write_urb_pool[i]);
+		}
 	for (i = 0; i < NUM_CONTROL_URBS; ++i) 
-		if (bluetooth->control_urb_pool[i])
+		if (bluetooth->control_urb_pool[i]) {
+			if (bluetooth->control_urb_pool[i]->transfer_buffer)
+				kfree (bluetooth->control_urb_pool[i]->transfer_buffer);
 			usb_free_urb (bluetooth->control_urb_pool[i]);
+		}
 
 	bluetooth_table[minor] = NULL;
 

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[Andi Kleen]

"David S. Miller" <davem@redhat.com> writes:

> Dawson Engler writes:
>  > Is there any way to automatically find these?  E.g., is any routine
>  > with "asmlinkage" callable from user space?
> 
> This is only universally done in generic and x86 specific code,
> other ports tend to forget asmlinkage simply because most ports
> don't need it.

Even i386 doesn't need it because the stack frame happens to have the
right order of the arguments at the right position. Just you can get into 
weird bugs when any function modifies their argument because it'll be still 
modified after syscall restart but only depending if the compiler used a 
temporary register or not.

-Andi

Re: [CHECKER] 4 security holes in 2.4.4-ac8

[Keith Owens]

On 30 May 2001 11:38:13 +0200, 
Andi Kleen <ak@suse.de> wrote:
>"David S. Miller" <davem@redhat.com> writes:
>
>> Dawson Engler writes:
>>  > Is there any way to automatically find these?  E.g., is any routine
>>  > with "asmlinkage" callable from user space?
>> 
>> This is only universally done in generic and x86 specific code,
>> other ports tend to forget asmlinkage simply because most ports
>> don't need it.
>
>Even i386 doesn't need it because the stack frame happens to have the
>right order of the arguments at the right position. Just you can get into 
>weird bugs when any function modifies their argument because it'll be still 
>modified after syscall restart but only depending if the compiler used a 
>temporary register or not.

For IA64 you *must* use asmlinkage because the first 8 parameters are
passed in registers.  gcc will happily modify the register values which
will mess up syscall restart, unless you use asmlinkage to force gcc to
take copies of parameters and modify the copies.