From da65538369cf80e4559f8ef86db7c0913b222890 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Sat, 29 Aug 2009 20:46:41 -0700 Subject: [PATCH] Kernel - replace unbounded uses of kvcprintf() and reduce stack use by devfs * Replace unbounded uses of kvcprintf() to guarantee that buffers do not overflow. * Do not declare PATH_MAX buffers on the stack. Use kvasnrprintf() or kmalloc() to allocate space. * In make_autoclone_dev() fix an improper use of a buffer passed as the fmt argument to make_dev(). --- sys/dev/raid/vinum/vinumconfig.c | 14 +----- sys/kern/kern_conf.c | 53 ++++++-------------- sys/sys/devfs.h | 1 - sys/vfs/devfs/devfs_core.c | 103 +++++++++++++++------------------------ 4 files changed, 58 insertions(+), 113 deletions(-) diff --git a/sys/dev/raid/vinum/vinumconfig.c b/sys/dev/raid/vinum/vinumconfig.c index fd55314033..3b926f3dfa 100644 --- a/sys/dev/raid/vinum/vinumconfig.c +++ b/sys/dev/raid/vinum/vinumconfig.c @@ -97,7 +97,6 @@ struct putchar_arg { void throw_rude_remark(int error, char *msg,...) { - int retval; __va_list ap; char *text; static int finishing; /* don't recurse */ @@ -112,19 +111,10 @@ throw_rude_remark(int error, char *msg,...) * We can't just format to ioctl_reply, since it * may contain our input parameters */ - text = Malloc(MSG_MAX); - if (text == NULL) { - log(LOG_ERR, "vinum: can't allocate error message buffer\n"); - kprintf("vinum: "); - kvprintf(msg, ap); /* print to the console */ - kprintf("\n"); - } else { - retval = kvcprintf(msg, NULL, (void *) text, 10, ap); - text[retval] = '\0'; /* delimit */ + kvasnrprintf(&text, MSG_MAX, 10, msg, ap); strcpy(ioctl_reply->msg, text); ioctl_reply->error = error; /* first byte is the error number */ - Free(text); - } + kvasfree(&text); } else { kprintf("vinum: "); kvprintf(msg, ap); /* print to the console */ diff --git a/sys/kern/kern_conf.c b/sys/kern/kern_conf.c index bf9a9f6c9f..c9f458535a 100644 --- a/sys/kern/kern_conf.c +++ b/sys/kern/kern_conf.c @@ -180,32 +180,18 @@ make_dev(struct dev_ops *ops, int minor, uid_t uid, gid_t gid, { cdev_t devfs_dev; __va_list ap; - int i; - char dev_name[PATH_MAX+1]; /* * compile the cdevsw and install the device */ compile_dev_ops(ops); - /* - * Set additional fields (XXX DEVFS interface goes here) - */ + devfs_dev = devfs_new_cdev(ops, minor); __va_start(ap, fmt); - i = kvcprintf(fmt, NULL, dev_name, 32, ap); - dev_name[i] = '\0'; + kvsnrprintf(devfs_dev->si_name, sizeof(devfs_dev->si_name), + 32, fmt, ap); __va_end(ap); -/* - if ((devfs_dev = devfs_find_device_by_name(dev_name)) != NULL) { - kprintf("make_dev: Device %s already exists, returning old dev without creating new node\n", dev_name); - return devfs_dev; - } -*/ - - devfs_dev = devfs_new_cdev(ops, minor); - memcpy(devfs_dev->si_name, dev_name, i+1); - devfs_debug(DEVFS_DEBUG_INFO, "make_dev called for %s\n", devfs_dev->si_name); @@ -221,8 +207,6 @@ make_only_devfs_dev(struct dev_ops *ops, int minor, uid_t uid, gid_t gid, { cdev_t devfs_dev; __va_list ap; - int i; - //char *dev_name; /* * compile the cdevsw and install the device @@ -234,11 +218,10 @@ make_only_devfs_dev(struct dev_ops *ops, int minor, uid_t uid, gid_t gid, * Set additional fields (XXX DEVFS interface goes here) */ __va_start(ap, fmt); - i = kvcprintf(fmt, NULL, devfs_dev->si_name, 32, ap); - devfs_dev->si_name[i] = '\0'; + kvsnrprintf(devfs_dev->si_name, sizeof(devfs_dev->si_name), + 32, fmt, ap); __va_end(ap); - devfs_create_dev(devfs_dev, uid, gid, perms); return (devfs_dev); @@ -251,8 +234,6 @@ make_only_dev(struct dev_ops *ops, int minor, uid_t uid, gid_t gid, { cdev_t devfs_dev; __va_list ap; - int i; - //char *dev_name; /* * compile the cdevsw and install the device @@ -267,8 +248,8 @@ make_only_dev(struct dev_ops *ops, int minor, uid_t uid, gid_t gid, * Set additional fields (XXX DEVFS interface goes here) */ __va_start(ap, fmt); - i = kvcprintf(fmt, NULL, devfs_dev->si_name, 32, ap); - devfs_dev->si_name[i] = '\0'; + kvsnrprintf(devfs_dev->si_name, sizeof(devfs_dev->si_name), + 32, fmt, ap); __va_end(ap); reference_dev(devfs_dev); @@ -326,16 +307,15 @@ sync_devs(void) int make_dev_alias(cdev_t target, const char *fmt, ...) { - char name[PATH_MAX + 1]; __va_list ap; - int i; + char *name; __va_start(ap, fmt); - i = kvcprintf(fmt, NULL, name, 32, ap); - name[i] = '\0'; + kvasnrprintf(&name, PATH_MAX, 32, fmt, ap); __va_end(ap); devfs_make_alias(name, target); + kvasfree(&name); return 0; } @@ -346,22 +326,21 @@ cdev_t make_autoclone_dev(struct dev_ops *ops, struct devfs_bitmap *bitmap, d_clone_t *nhandler, uid_t uid, gid_t gid, int perms, const char *fmt, ...) { - cdev_t dev; - char name[PATH_MAX + 1]; __va_list ap; - int i; + cdev_t dev; + char *name; __va_start(ap, fmt); - i = kvcprintf(fmt, NULL, name, 32, ap); - name[i] = '\0'; + kvasnrprintf(&name, PATH_MAX, 32, fmt, ap); __va_end(ap); if (bitmap != NULL) devfs_clone_bitmap_init(bitmap); devfs_clone_handler_add(name, nhandler); - dev = make_dev(&default_dev_ops, 0xffff00ff, uid, gid, perms, name); - + dev = make_dev(&default_dev_ops, 0xffff00ff, + uid, gid, perms, "%s", name); + kvasfree(&name); return dev; } diff --git a/sys/sys/devfs.h b/sys/sys/devfs.h index 5f28699a42..ebd5967be7 100644 --- a/sys/sys/devfs.h +++ b/sys/sys/devfs.h @@ -411,7 +411,6 @@ int devfs_apply_rules(char *); int devfs_reset_rules(char *); int devfs_scan_callback(devfs_scan_t *); -int devfs_node_to_path(struct devfs_node *, char *); int devfs_clr_subnames_flag(char *, uint32_t); int devfs_destroy_subnames_without_flag(char *, uint32_t); diff --git a/sys/vfs/devfs/devfs_core.c b/sys/vfs/devfs/devfs_core.c index d369e219a8..4a38783b59 100644 --- a/sys/vfs/devfs/devfs_core.c +++ b/sys/vfs/devfs/devfs_core.c @@ -710,16 +710,14 @@ devfs_find_device_by_name(const char *fmt, ...) { cdev_t found = NULL; devfs_msg_t msg; - char target[PATH_MAX+1]; + char *target; __va_list ap; - int i; if (fmt == NULL) return NULL; __va_start(ap, fmt); - i = kvcprintf(fmt, NULL, target, 10, ap); - target[i] = '\0'; + kvasnrprintf(&target, PATH_MAX, 10, fmt, ap); __va_end(ap); msg = devfs_msg_get(); @@ -727,6 +725,7 @@ devfs_find_device_by_name(const char *fmt, ...) msg = devfs_msg_send_sync(DEVFS_FIND_DEVICE_BY_NAME, msg); found = msg->mdv_cdev; devfs_msg_put(msg); + kvasfree(&target); return found; } @@ -1538,10 +1537,13 @@ devfs_alias_create(char *name_orig, struct devfs_node *target, int rule_based) struct devfs_node *parent = DEVFS_MNTDATA(mp)->root_node; struct devfs_node *linknode; char *create_path = NULL; - char *name, name_buf[PATH_MAX]; + char *name; + char *name_buf; + int result = 0; KKASSERT((lockstatus(&devfs_lock, curthread)) == LK_EXCLUSIVE); + name_buf = kmalloc(PATH_MAX, M_TEMP, M_WAITOK); devfs_resolve_name_path(name_orig, name_buf, &create_path, &name); if (create_path) @@ -1553,13 +1555,15 @@ devfs_alias_create(char *name_orig, struct devfs_node *target, int rule_based) "Node already exists: %s " "(devfs_make_alias_worker)!\n", name); - return 1; + result = 1; + goto done; } - linknode = devfs_allocp(Plink, name, parent, mp, NULL); - if (linknode == NULL) - return 1; + if (linknode == NULL) { + result = 1; + goto done; + } linknode->link_target = target; target->nlinks++; @@ -1567,7 +1571,9 @@ devfs_alias_create(char *name_orig, struct devfs_node *target, int rule_based) if (rule_based) linknode->flags |= DEVFS_RULE_CREATED; - return 0; +done: + kfree(name_buf, M_TEMP); + return (result); } /* @@ -1654,27 +1660,32 @@ struct devfs_node * devfs_resolve_or_create_path(struct devfs_node *parent, char *path, int create) { struct devfs_node *node = parent; - char buf[PATH_MAX]; + char *buf; size_t idx = 0; - if (path == NULL) return parent; + buf = kmalloc(PATH_MAX, M_TEMP, M_WAITOK); - for (; *path != '\0' ; path++) { + while (*path && idx < PATH_MAX - 1) { if (*path != '/') { buf[idx++] = *path; } else { buf[idx] = '\0'; node = devfs_resolve_or_create_dir(node, buf, idx, create); - if (node == NULL) + if (node == NULL) { + kfree(buf, M_TEMP); return NULL; + } idx = 0; } + ++path; } buf[idx] = '\0'; - return devfs_resolve_or_create_dir(node, buf, idx, create); + node = devfs_resolve_or_create_dir(node, buf, idx, create); + kfree (buf, M_TEMP); + return (node); } /* @@ -1729,19 +1740,18 @@ devfs_create_device_node(struct devfs_node *root, cdev_t dev, { struct devfs_node *parent, *node = NULL; char *path = NULL; - char *name, name_buf[PATH_MAX]; + char *name; + char *name_buf; __va_list ap; int i, found; - char *create_path = NULL; char *names = "pqrsPQRS"; - if (path_fmt != NULL) { - path = kmalloc(PATH_MAX+1, M_DEVFS, M_WAITOK); + name_buf = kmalloc(PATH_MAX, M_TEMP, M_WAITOK); + if (path_fmt != NULL) { __va_start(ap, path_fmt); - i = kvcprintf(path_fmt, NULL, path, 10, ap); - path[i] = '\0'; + kvasnrprintf(&path, PATH_MAX, 10, path_fmt, ap); __va_end(ap); } @@ -1794,9 +1804,8 @@ devfs_create_device_node(struct devfs_node *root, cdev_t dev, } out: - if (path_fmt != NULL) - kfree(path, M_DEVFS); - + kfree(name_buf, M_TEMP); + kvasfree(&path); return node; } @@ -1865,12 +1874,14 @@ int devfs_destroy_device_node(struct devfs_node *root, cdev_t target) { struct devfs_node *node, *parent; - char *name, name_buf[PATH_MAX]; + char *name; + char *name_buf; char *create_path = NULL; KKASSERT(target); - memcpy(name_buf, target->si_name, strlen(target->si_name)+1); + name_buf = kmalloc(PATH_MAX, M_TEMP, M_WAITOK); + ksnprintf(name_buf, PATH_MAX, "%s", target->si_name); devfs_resolve_name_path(target->si_name, name_buf, &create_path, &name); @@ -1888,6 +1899,7 @@ devfs_destroy_device_node(struct devfs_node *root, cdev_t target) nanotime(&node->parent->mtime); devfs_gc(node); } + kfree(name_buf, M_TEMP); return 0; } @@ -1931,42 +1943,6 @@ devfs_propagate_dev(cdev_t dev, int attach) } /* - * devfs_node_to_path takes a node and a buffer of a size of - * at least PATH_MAX, resolves the full path from the root - * node and writes it in a humanly-readable format into the - * buffer. - * If DEVFS_STASH_DEPTH is less than the directory level up - * to the root node, only the last DEVFS_STASH_DEPTH levels - * of the path are resolved. - */ -int -devfs_node_to_path(struct devfs_node *node, char *buffer) -{ -#define DEVFS_STASH_DEPTH 32 - struct devfs_node *node_stash[DEVFS_STASH_DEPTH]; - int i, offset; - memset(buffer, 0, PATH_MAX); - - for (i = 0; (i < DEVFS_STASH_DEPTH) && (node->node_type != Proot); i++) { - node_stash[i] = node; - node = node->parent; - } - i--; - - for (offset = 0; i >= 0; i--) { - memcpy(buffer+offset, node_stash[i]->d_dir.d_name, - node_stash[i]->d_dir.d_namlen); - offset += node_stash[i]->d_dir.d_namlen; - if (i > 0) { - *(buffer+offset) = '/'; - offset++; - } - } -#undef DEVFS_STASH_DEPTH - return 0; -} - -/* * devfs_clone either returns a basename from a complete name by * returning the length of the name without trailing digits, or, * if clone != 0, calls the device's clone handler to get a new @@ -2094,9 +2070,10 @@ cdev_t devfs_new_cdev(struct dev_ops *ops, int minor) { cdev_t dev = sysref_alloc(&cdev_sysref_class); + sysref_activate(&dev->si_sysref); reference_dev(dev); - memset(dev, 0, offsetof(struct cdev, si_sysref)); + bzero(dev, offsetof(struct cdev, si_sysref)); dev->si_uid = 0; dev->si_gid = 0; -- 2.11.4.GIT