lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 24 Jul 2007 13:57:53 +0200 From: Alban Crequy <alban.crequy@...nodes.com> To: jens.axboe@...cle.com Cc: linux-kernel@...r.kernel.org Subject: [RFC] error management in add_disk() Hi, I have a problem with the error management of add_disk() and del_gendisk(). add_disk() adds an entry in /sys/block/<name>. The filename in /sys/block is not (struct gen_disk)->disk_name but more or less the first KOBJ_NAME_LEN characters of (struct gen_disk)->disk_name. #define KOBJ_NAME_LEN 20 My problem occurs when we try to add 2 disks with different names, but when the KOBJ_NAME_LEN first characters are the same. It is not allowed to have several files in /sys/block/ with the same name. It does not prevent the disk to work, but the creation of the file in /sys/block will silently fail. See the call chain: add_disk() -> register_disk() -> kobject_add(&disk->kobj) del_gendisk() -> kobject_del(&disk->kobj) add_disk() and register_disk() return void, so the caller cannot know that there is a problem. kobject_add() returns an error but register_disk() ignores the error. The disk driver is unaware of the failure of disk_add(), and may call del_gendisk(). It deletes an object in /sys/block/ that was not added (bug!). The disk driver cannot check that, so the reference counter of /sys/block is decremented on kobject_del(). If the user run add_disk() and del_gendisk() a lot of times with different names having the same 20-characters beginning, the reference counter of the /sys/block directory will reach 0 (and it will be freed) although it still contains files. The attached test module triggers the problem. You can try something like: for i in $(seq 1 100) ; do insmod ./adddiskbug.ko ; rmmod adddiskbug ; done The attached patch fixes the problem by changing the prototype of add_disk() and register_disk() to return errors. Index: linux-2.6.23-rc1/block/genhd.c =================================================================== --- linux-2.6.23-rc1.orig/block/genhd.c 2007-07-23 14:52:11.000000000 +0200 +++ linux-2.6.23-rc1/block/genhd.c 2007-07-23 18:08:58.000000000 +0200 @@ -175,13 +175,22 @@ * This function registers the partitioning information in @disk * with the kernel. */ -void add_disk(struct gendisk *disk) +int add_disk(struct gendisk *disk) { + int ret; + disk->flags |= GENHD_FL_UP; blk_register_region(MKDEV(disk->major, disk->first_minor), disk->minors, NULL, exact_match, exact_lock, disk); - register_disk(disk); + ret = register_disk(disk); + if (ret) { + blk_unregister_region(MKDEV(disk->major, disk->first_minor), + disk->minors); + return ret; + } blk_register_queue(disk); + + return 0; } EXPORT_SYMBOL(add_disk); Index: linux-2.6.23-rc1/include/linux/genhd.h =================================================================== --- linux-2.6.23-rc1.orig/include/linux/genhd.h 2007-07-09 01:32:17.000000000 +0200 +++ linux-2.6.23-rc1/include/linux/genhd.h 2007-07-23 15:12:53.000000000 +0200 @@ -235,7 +235,7 @@ /* drivers/block/genhd.c */ extern int get_blkdev_list(char *, int); -extern void add_disk(struct gendisk *disk); +extern int add_disk(struct gendisk *disk); extern void del_gendisk(struct gendisk *gp); extern void unlink_gendisk(struct gendisk *gp); extern struct gendisk *get_gendisk(dev_t dev, int *part); Index: linux-2.6.23-rc1/fs/partitions/check.c =================================================================== --- linux-2.6.23-rc1.orig/fs/partitions/check.c 2007-07-23 14:52:13.000000000 +0200 +++ linux-2.6.23-rc1/fs/partitions/check.c 2007-07-23 15:53:04.000000000 +0200 @@ -469,7 +469,7 @@ } /* Not exported, helper to add_disk(). */ -void register_disk(struct gendisk *disk) +int register_disk(struct gendisk *disk) { struct block_device *bdev; char *s; @@ -483,11 +483,11 @@ if (s) *s = '!'; if ((err = kobject_add(&disk->kobj))) - return; + return -EEXIST; err = disk_sysfs_symlinks(disk); if (err) { kobject_del(&disk->kobj); - return; + return -EEXIST; } disk_sysfs_add_subdirs(disk); @@ -523,6 +523,8 @@ continue; kobject_uevent(&p->kobj, KOBJ_ADD); } + + return 0; } int rescan_partitions(struct gendisk *disk, struct block_device *bdev) Index: linux-2.6.23-rc1/include/linux/blkdev.h =================================================================== --- linux-2.6.23-rc1.orig/include/linux/blkdev.h 2007-07-23 14:52:14.000000000 +0200 +++ linux-2.6.23-rc1/include/linux/blkdev.h 2007-07-23 15:58:47.000000000 +0200 @@ -643,7 +643,7 @@ extern int blk_register_queue(struct gendisk *disk); extern void blk_unregister_queue(struct gendisk *disk); -extern void register_disk(struct gendisk *dev); +extern int register_disk(struct gendisk *dev); extern void generic_make_request(struct bio *bio); extern void blk_put_request(struct request *); extern void __blk_put_request(request_queue_t *, struct request *); My test module: ----->8---------->8---------->8----- #include <linux/module.h> #include <linux/kernel.h> #include <linux/blkdev.h> #include <linux/genhd.h> #include <linux/fs.h> #include <linux/init.h> #define DRIVER_NAME "adddiskbug" int adddiskbug_init(void); void adddiskbug_exit(void); struct block_device_operations adddiskbug_fops = { .owner = THIS_MODULE, .open = NULL, .release = NULL, .ioctl = NULL, }; int major_number; struct gendisk *gd[256]; void print_refcount(struct gendisk *_gd, const char *str) { struct kobject *obj; printk("%s\n", str); printk("name=%s: count=%d\n", _gd->disk_name, atomic_read(& _gd->kobj.kref.refcount)); for (obj = & _gd->kobj ; obj ; obj = obj->parent) { printk(" obj %p '%s': %p '%s' count=%d\n", obj, obj->name, obj->k_name, obj->k_name?obj->k_name:"", atomic_read(& obj->kref.refcount)); } } int new_disk(const char *name, int minor) { int ret = 0; if (strlen(name) >= 32) { printk("ERROR: Name '%s' too long\n", name); return -EINVAL; } gd[minor] = alloc_disk(1); if (!gd[minor]) return -ENOMEM; print_refcount(gd[0], "After alloc_disk:\n"); gd[minor]->major = major_number; gd[minor]->first_minor = minor; gd[minor]->fops = &adddiskbug_fops; strlcpy(gd[minor]->disk_name, name, sizeof(gd[minor]->disk_name)); set_capacity(gd[minor], (sector_t) 8192); /* 4MB */ gd[minor]->queue = blk_alloc_queue(GFP_KERNEL); if (! gd[minor]->queue) { put_disk(gd[minor]); return -ENOMEM; } #ifndef KERNEL_WITH_MY_PATCH add_disk(gd[minor]); #else ret = add_disk(gd[minor]); if (ret) { blk_cleanup_queue(gd[minor]->queue); put_disk(gd[minor]); return ret; } #endif return ret; } void delete_disk(int minor) { struct gendisk *_gd = gd[minor]; blk_cleanup_queue(_gd->queue); del_gendisk(_gd); put_disk(_gd); } int adddiskbug_init(void) { int ret; printk(" ==== init ===== \n"); major_number = register_blkdev(0, DRIVER_NAME); if (major_number < 0) return -EINVAL; ret = new_disk("my_bogus_driver_number_1", 0); if (ret < 0) { unregister_blkdev(major_number, DRIVER_NAME); return -EINVAL; } print_refcount(gd[0], "after new_disk 0"); ret = new_disk("my_bogus_driver_number_2", 1); if (ret < 0) { delete_disk(0); unregister_blkdev(major_number, DRIVER_NAME); return -EINVAL; } print_refcount(gd[0], "after new_disk 1"); return 0; } void adddiskbug_exit(void) { print_refcount(gd[0], "before delete_disk 0"); delete_disk(1); print_refcount(gd[0], "before delete_disk 1"); delete_disk(0); unregister_blkdev(major_number, DRIVER_NAME); } module_init(adddiskbug_init); module_exit(adddiskbug_exit); ----->8---------->8---------->8----- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists