Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy
flat view
From: Nicolai Stange <hidden>
Date: 2019-11-21 12:18:17
Also in:
linux-crypto, lkml
Hi Stephan, two general remarks on debugfs usage below Stephan Müller [off-list ref] writes:
quoted hunk
diff --git a/drivers/char/lrng/lrng_testing.c b/drivers/char/lrng/lrng_testing.c new file mode 100644 index 000000000000..5c33d3bd2172 --- /dev/null +++ b/drivers/char/lrng/lrng_testing.c
<snip>
quoted hunk
+/* + * This data structure holds the dentry's of the debugfs files establishing + * the interface to user space. + */ +struct lrng_raw_debugfs { + struct dentry *lrng_raw_debugfs_root; /* root dentry */ + struct dentry *lrng_raw_debugfs_lrng_raw; /* .../lrng_raw */ +}; + +static struct lrng_raw_debugfs lrng_raw_debugfs; + +/* DebugFS operations and definition of the debugfs files */ +static ssize_t lrng_raw_read(struct file *file, char __user *to, + size_t count, loff_t *ppos) +{ + loff_t pos = *ppos; + int ret; + + if (!count) + return 0; + lrng_raw_entropy_init(); + ret = lrng_raw_extract_user(to, count); + lrng_raw_entropy_fini(); + if (ret < 0) + return ret; + count -= ret; + *ppos = pos + count; + return ret; +} + +/* Module init: allocate memory, register the debugfs files */ +static int lrng_raw_debugfs_init(void) +{ + lrng_raw_debugfs.lrng_raw_debugfs_root = + debugfs_create_dir(KBUILD_MODNAME, NULL); + if (IS_ERR(lrng_raw_debugfs.lrng_raw_debugfs_root)) { + lrng_raw_debugfs.lrng_raw_debugfs_root = NULL; + return PTR_ERR(lrng_raw_debugfs.lrng_raw_debugfs_root); + }
I think pointers returned by the debugfs API are not supposed to get
checked for NULL/IS_ERR(), c.f commit ff9fb72bc077 ("debugfs: return
error values, not NULL") or the the output from
git log --pretty=oneline | grep 'no need to check return value of debugfs_create'
(Also the above code is dubious: you're effectively returning
PTR_ERR(NULL)).
quoted hunk
+ return 0; +} + +static struct file_operations lrng_raw_name_fops = { + .owner = THIS_MODULE, + .read = lrng_raw_read, +}; + +static int lrng_raw_debugfs_init_name(void) +{ + lrng_raw_debugfs.lrng_raw_debugfs_lrng_raw = + debugfs_create_file("lrng_raw", 0400, + lrng_raw_debugfs.lrng_raw_debugfs_root, + NULL, &lrng_raw_name_fops);q
CONFIG_LRNG_TESTING is a bool and thus, this debugfs file can't ever get removed. Even if it could, this inode hasn't got any data associated with it and so file removal would not be a problem for lrng_raw_read(). Please consider using debugfs_create_file_unsafe() instead to save debugfs from kmalloc()ing a proxy file_operations protecting your fops against concurrent file removal.
quoted hunk
+ if (IS_ERR(lrng_raw_debugfs.lrng_raw_debugfs_lrng_raw)) { + lrng_raw_debugfs.lrng_raw_debugfs_lrng_raw = NULL; + return PTR_ERR(lrng_raw_debugfs.lrng_raw_debugfs_lrng_raw); + }
Same comment regarding return value checking applies here. Thanks, Nicolai
quoted hunk
+ return 0; +} + +static int __init lrng_raw_init(void) +{ + int ret = lrng_raw_debugfs_init(); + + if (ret < 0) + return ret; + + ret = lrng_raw_debugfs_init_name(); + if (ret < 0) + debugfs_remove_recursive( + lrng_raw_debugfs.lrng_raw_debugfs_root); + + return ret; +} + +static void __exit lrng_raw_exit(void) +{ + debugfs_remove_recursive(lrng_raw_debugfs.lrng_raw_debugfs_root); +} + +module_init(lrng_raw_init); +module_exit(lrng_raw_exit); + +MODULE_LICENSE("Dual BSD/GPL"); +MODULE_AUTHOR("Stephan Mueller <smueller@chronox.de>"); +MODULE_DESCRIPTION("Kernel module for gathering raw entropy");
-- SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg), GF: Felix Imendörffer