| 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
| ||
|
Message-ID: <23500762.5LfuSYdpsK@positron.chronox.de>
Date: Thu, 21 Nov 2019 16:18:18 +0100
From: Stephan Müller <smueller@...onox.de>
To: Nicolai Stange <nstange@...e.de>
Cc: Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-crypto@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
linux-api@...r.kernel.org,
"Eric W. Biederman" <ebiederm@...ssion.com>,
"Alexander E. Patrakov" <patrakov@...il.com>,
"Ahmed S. Darwish" <darwish.07@...il.com>,
"Theodore Y. Ts'o" <tytso@....edu>, Willy Tarreau <w@....eu>,
Matthew Garrett <mjg59@...f.ucam.org>,
Vito Caputo <vcaputo@...garu.com>,
Andreas Dilger <adilger.kernel@...ger.ca>,
Jan Kara <jack@...e.cz>, Ray Strode <rstrode@...hat.com>,
William Jon McCann <mccann@....edu>,
zhangjs <zachary@...shancloud.com>,
Andy Lutomirski <luto@...nel.org>,
Florian Weimer <fweimer@...hat.com>,
Lennart Poettering <mzxreary@...inter.de>,
"Peter, Matthias" <matthias.peter@....bund.de>,
Marcelo Henrique Cerri <marcelo.cerri@...onical.com>,
Roman Drahtmueller <draht@...altsekun.de>,
Neil Horman <nhorman@...hat.com>
Subject: Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy
Am Donnerstag, 21. November 2019, 13:18:10 CET schrieb Nicolai Stange:
Hi Nicolai,
> Hi Stephan,
>
> two general remarks on debugfs usage below
>
> Stephan Müller <smueller@...onox.de> writes:
> > 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>
>
> > +/*
> > + * 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)).
Removed the check compliant to the mentioned patches.
>
> > + 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().
Correct.
>
> 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.
Yes, you are correct. Changed.
>
> > + 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.
Same here: I removed the check.
With that, I also removed the static variable to maintain the two dentries
following the examples seen in other kernel code. Also, the __exit function is
removed as we do not need it as you pointed out.
Thanks a lot.
>
> Thanks,
>
> Nicolai
>
> > + 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@...onox.de>");
> > +MODULE_DESCRIPTION("Kernel module for gathering raw entropy");
Ciao
Stephan
Powered by blists - more mailing lists