linux-kernel - Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210728012740.GA90475@rocinante>
Date:   Wed, 28 Jul 2021 03:27:40 +0200
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Amey Narkhede <ameynarkhede03@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        alex.williamson@...hat.com,
        Raphael Norwitz <raphael.norwitz@...anix.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Shanker Donthineni <sdonthineni@...dia.com>,
        Sinan Kaya <okaya@...nel.org>, Len Brown <lenb@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set
 device reset mechanism

Hi Bjorn,

[...]
> > +	if (count >= (PAGE_SIZE - 1))
> > +		return -EINVAL;
> 
> I'm not the sysfs expert, but surely the sysfs infrastructure already
> guarantees this?

We don't need to store any value, since we are processing the input from
the userspace, thus ensuring that we have room for the newline is not
needed, especially since the show() function dynamically builds the
content to show, so indeed this check can be dropped.

To add, there aren't any guarantees other from sysfs than we get a up to
a PAGE_SIZE worth of data in the buffer.

[...]
> > +	options = kstrndup(buf, count, GFP_KERNEL);
> 
> I assume the kstrndup() is because strsep() writes into the buffer?

Yes, Amey added kstrndup() in v6 following my recommendation as per:

  https://lore.kernel.org/linux-pci/20210606125800.GA76573@rocinante.localdomain/

This was to avoid removing the const quantifier through a type cast
given that the signature of the function denotes that the buffer is
a pointer to immutable string, as per:

  https://elixir.bootlin.com/linux/v5.14-rc3/source/include/linux/device/driver.h#L137

Some other sysfs users do employ the cast when using strtok() and I am
not so such it's the right way to do it, as per:

  drivers/s390/net/qeth_l3_sys.c
  232:	tmp = strsep((char **)&buf, "\n");

  drivers/media/rc/rc-main.c
  1167:	while ((tmp = strsep((char **)&buf, " \n")) != NULL) {

> Aren't we allowed to write into the buffer we get from sysfs?  Does
> the user ever see the buffer contents again?  I would think sysfs
> would have already done a copy_from_user() or whatever.

I might be wrong about this, but I suppose this might be to stop people
from accidentally freeing the buffer as kernfs_fop_write_iter() would do
it after all the internal housekeeping is done, provided that someone
pays attention to compile time warnings.

	Krzysztof