Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756554AbdIHP32 (ORCPT ); Fri, 8 Sep 2017 11:29:28 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:47980 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755916AbdIHP30 (ORCPT ); Fri, 8 Sep 2017 11:29:26 -0400 Date: Fri, 8 Sep 2017 17:29:23 +0200 From: Greg Kroah-Hartman To: Vivien Didelot Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com, "David S. Miller" , Florian Fainelli , Andrew Lunn , Egil Hjelmeland , John Crispin , Woojung Huh , Sean Wang , Nikita Yushchenko , Chris Healy Subject: Re: [PATCH net-next v2 02/10] net: dsa: debugfs: add tree Message-ID: <20170908152923.GA19672@kroah.com> References: <20170828191748.19492-1-vivien.didelot@savoirfairelinux.com> <20170828191748.19492-3-vivien.didelot@savoirfairelinux.com> <87k21944rq.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87k21944rq.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me> User-Agent: Mutt/1.9.0 (2017-09-02) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4565 Lines: 173 On Fri, Sep 08, 2017 at 10:57:29AM -0400, Vivien Didelot wrote: > Hi Greg, > > You wrote: > > > > Can I ask for a quick review of this patch as well? It's the one adding > > > the boilerplate for a single debugfs file, and I'm pretty sure it can be > > > reduced somehow. > > > > I don't see a patch here :( > > Oops, you weren't originally in Cc. Please find the patch below. > > > > Also more important, you will notice what seems to be a bug to me: > > > I can read or write a file even if I didn't mask the corresponding mode > > > hence the double check in dsa_debugfs_show and dsa_debugfs_write. > > > > The mode can be changed by userspace, you shouldn't ever need to check > > it in any debugfs calls, right? > > Correct. But this happens even if the file mode isn't changed by > userspace in the meantime, which seemed weird to me. e.g. echo > redirected to a -r--r--r-- debugfs entry will call dsa_debugfs_write. > > > Thanks, > > Vivien > > > ------ Beginning of the patch ------ > > This commit adds the boiler plate to create a DSA related debug > filesystem entry as well as a "tree" file, containing the tree index. > > # cat switch1/tree > 0 > > Signed-off-by: Vivien Didelot > Reviewed-by: Florian Fainelli > Reviewed-by: Andrew Lunn > --- > net/dsa/debugfs.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 107 insertions(+) > > diff --git a/net/dsa/debugfs.c b/net/dsa/debugfs.c > index b6b5e5c97389..54e97e05a9d7 100644 > --- a/net/dsa/debugfs.c > +++ b/net/dsa/debugfs.c > @@ -10,6 +10,7 @@ > */ > > #include > +#include > > #include "dsa_priv.h" > > @@ -19,6 +20,107 @@ > /* DSA module debugfs directory */ > static struct dentry *dsa_debugfs_dir; > > +struct dsa_debugfs_ops { > + int (*read)(struct dsa_switch *ds, int id, struct seq_file *seq); > + int (*write)(struct dsa_switch *ds, int id, char *buf); > +}; > + > +struct dsa_debugfs_priv { > + const struct dsa_debugfs_ops *ops; > + struct dsa_switch *ds; > + int id; > +}; > + > +static int dsa_debugfs_show(struct seq_file *seq, void *p) > +{ > + struct dsa_debugfs_priv *priv = seq->private; > + struct dsa_switch *ds = priv->ds; > + > + /* Somehow file mode is bypassed... Double check here */ As was said, root can do this, change your comment, just delete it :) > + if (!priv->ops->read) > + return -EOPNOTSUPP; > + > + return priv->ops->read(ds, priv->id, seq); > +} > + > +static ssize_t dsa_debugfs_write(struct file *file, const char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct seq_file *seq = file->private_data; > + struct dsa_debugfs_priv *priv = seq->private; > + struct dsa_switch *ds = priv->ds; > + char buf[count + 1]; Nice, userspace asks to write 100Gb, and boom, you just smashed the stack! Repeat after me: All input is evil. Say it again. Always remember it. > + int err; > + > + /* Somehow file mode is bypassed... Double check here */ > + if (!priv->ops->write) > + return -EOPNOTSUPP; > + > + if (copy_from_user(buf, user_buf, count)) > + return -EFAULT; > + > + buf[count] = '\0'; Be careful here. Use the kernel library functions instead of a "raw" copy_from/to_user() calls, that is what they are there for (simple_read_to_buffer, simple_write_to_buffer). > + > + err = priv->ops->write(ds, priv->id, buf); > + > + return err ? err : count; > +} > + > +static int dsa_debugfs_open(struct inode *inode, struct file *file) > +{ > + return single_open(file, dsa_debugfs_show, inode->i_private); > +} > + > +static const struct file_operations dsa_debugfs_fops = { > + .open = dsa_debugfs_open, > + .read = seq_read, > + .write = dsa_debugfs_write, > + .llseek = no_llseek, > + .release = single_release, > + .owner = THIS_MODULE, > +}; > + > +static int dsa_debugfs_create_file(struct dsa_switch *ds, struct dentry *dir, > + char *name, int id, > + const struct dsa_debugfs_ops *ops) > +{ > + struct dsa_debugfs_priv *priv; > + struct dentry *entry; > + umode_t mode; > + > + priv = devm_kzalloc(ds->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->ops = ops; > + priv->ds = ds; > + priv->id = id; > + > + mode = 0; > + if (ops->read) > + mode |= 0444; > + if (ops->write) > + mode |= 0200; > + > + entry = debugfs_create_file(name, mode, dir, priv, &dsa_debugfs_fops); > + if (IS_ERR_OR_NULL(entry)) > + return -EFAULT; Again, you don't care, don't check! thanks, greg k-h