[PATCH] selinux: fix dentry_open() error check

[PATCH] selinux: fix dentry_open() error check

[Akinobu Mita]

The return value of dentry_open() shoud be checked by IS_ERR().

Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: James Morris <jmorris@namei.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

---
 security/selinux/hooks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: work-fault-inject/security/selinux/hooks.c
===================================================================
--- work-fault-inject.orig/security/selinux/hooks.c
+++ work-fault-inject/security/selinux/hooks.c
@@ -1760,7 +1760,8 @@ static inline void flush_unauthorized_fi
 						get_file(devnull);
 					} else {
 						devnull = dentry_open(dget(selinux_null), mntget(selinuxfs_mount), O_RDWR);
-						if (!devnull) {
+						if (IS_ERR(devnull)) {
+							devnull = NULL;
 							put_unused_fd(fd);
 							fput(file);
 							continue;

Re: [PATCH] selinux: fix dentry_open() error check

[Christoph Hellwig]

On Mon, Nov 27, 2006 at 03:16:48PM +0900, Akinobu Mita wrote:
> The return value of dentry_open() shoud be checked by IS_ERR().

first great work finding all these calling convetion mismatches.

Do you have some tool to find these?

I wonder whether we should have some form of sparse annotation to
tell that a function returns these kinds of errors and we either
need to check IS_ERR or returned it again a caller with the same attribute.

Re: [PATCH] selinux: fix dentry_open() error check

[Akinobu Mita]

On Mon, Nov 27, 2006 at 06:35:58AM +0000, Christoph Hellwig wrote:
> On Mon, Nov 27, 2006 at 03:16:48PM +0900, Akinobu Mita wrote:
> > The return value of dentry_open() shoud be checked by IS_ERR().
> 
> first great work finding all these calling convetion mismatches.
> 
> Do you have some tool to find these?

Yes. I have just simple checker using regular expression.
It has the list of function name that need to check IS_ERR().
If there is not IS_ERR() within 3 line after calling the function in that list.
It print file name and line number.

$ find . -name '*.c' -not -name '*.mod.c' -exec checker.py {} \;
./net/bluetooth/rfcomm/tty.c: 264
./net/ipv6/ah6.c: 453
./net/sunrpc/pmap_clnt.c: 297
...

$ cat checker.py
#!/usr/bin/python
import sys
import re

def match_is_err(line):
	if re.search('IS_ERR', line):
		return True
	return False

funcs = [
	'lookup_instantiate_filp',
##	kernel
	'clockevent_set_next_event',
	'clk_get',
	'copy_process',
	'fork_idle',
	'kthread_create',
	'kthread_run',
	'kfifo_init',
	'kfifo_alloc',
	'ptrace_get_task_struct',
	'param_sysfs',
	'__stop_machine_run',

##	mm
	'do_brk',
	'do_mmap',
	'do_mmap_pgoff',
	'do_mremap',
	'follow_huge_addr',
	'mpol_copy',
	'nd_get_link',
	'read_cache_page',
	'read_mapping_page',
	'shmem_file_setup',
	'strndup_user',
	'sys_mremap',
	'__mpol_copy',

##	lib
	'alloc_ts_config',

##	block

##	crypto
	'crypto_alloc_base',
	'crypto_alloc_blkcipher',
	'crypto_alloc_cipher',
	'crypto_alloc_comp',
	'crypto_alloc_hash',
	'crypto_alloc_instance',
	'crypto_alg_mod_lookup',
	'crypto_lookup_template',
	'crypto_spawn_tfm',
	'crypto_get_attr_alg',
	'__crypto_alloc_tfm',

##	ipc
	'load_msg',

##	net
	'addrconf_dst_alloc',
	'ipv6_add_addr',
	'ipv6_renew_options',
	'netlink_getsockbyfilp',
	'rpcauth_create',
	'rpc_get_mount',
	'rpc_lookup_create',
	'skb_gso_segment',
	'skb_segment',
	'xt_find_match',
	'xt_find_table_lock',
	'xt_find_target',
	'__neigh_lookup_errno',

##	security
	'keyring_alloc',
	'key_type_lookup',
	'request_key_auth_new',

##	fs/*.c
	'bio_copy_user'
	'bio_map_kern'
	'bio_map_user'
	'bio_map_user_iov'
	'create_read_pipe',
	'create_write_pipe',
	'd_path',
	'dentry_open',
	'do_kern_mount',
	'ext3_journal_start',
	'ext3_journal_start_sb',
	'ext4_journal_start',
	'ext4_journal_start_sb',
	'filp_open',
	'freeze_bdev',
	'getname',
	'inotify_init',
	'jbd2_journal_start',
	'journal_start',
	'kern_mount',
	'lookup_bdev'
	'lookup_one_len',
	'mb_cache_entry_find_first',
	'mb_cache_entry_find_next',
	'nameidata_to_filp',
	'nd_get_link',
	'open_bdev_excl'
	'open_by_devnum',
	'open_exec',
	'posix_acl_from_xattr',
	'rpc_create',
	'sget',
	'simple_transaction_get',
	'vfs_kern_mount',

##	drivers/base/
	'class_create',
	'class_device_create',
	'device_create',
	'make_class_name',
	'platform_device_register_simple',

##	drivers
	'backlight_device_register',
	'drm_sysfs_device_add',
	'drm_sysfs_create',
	'hwmon_device_register',
	'rtc_device_register',
	'scsi_host_lookup',
	'tty_register_device',
	'__scsi_add_device',

##	misc
	'dma_mark_declared_memory_occupied',
]

prefix = '[^a-zA-Z_.>]'
postfix = '\s*\('

def match_funcs(line):
	if line[0] != '\t': # function declaration
		return False
		
	for func in funcs:
		if re.search(prefix + func + postfix, line):
			return True
	return False

def report(filename, lineno):
	print filename + ": " + str(lineno)

def check(filename):
	match = 0
	exceed = 0
	lineno = 0

	for line in open(filename, "r").readlines():
		lineno += 1

		if match_is_err(line):
			# reset
			match = 0
			exceed = 0

		if match > 0:
			exceed += 1
			if exceed > 3:
				report(filename, match)
				# reset
				match = 0
				exceed = 0
			

		if match_funcs(line) and not match_is_err(line):
			if match > 0:
				report(filename, match)

			# init
			match = lineno
			exceed = 0

	if match > 0:
		report(filename, match)

for arg in sys.argv[1:]:
	check(arg)


> I wonder whether we should have some form of sparse annotation to
> tell that a function returns these kinds of errors and we either
> need to check IS_ERR or returned it again a caller with the same attribute.

I really want this.

Re: [PATCH] selinux: fix dentry_open() error check

[James Morris]

On Mon, 27 Nov 2006, Akinobu Mita wrote:

> The return value of dentry_open() shoud be checked by IS_ERR().
> 
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: James Morris <jmorris@namei.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Thanks.

Applied to:

git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6#fixes



-- 
James Morris
<jmorris@namei.org>