linux-kernel - Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver

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]

Date:	Mon, 29 Sep 2014 12:14:25 +0200
From:	Antonios Motakis <a.motakis@...tualopensystems.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Alex Williamson <alex.williamson@...hat.com>,
	kvm-arm <kvmarm@...ts.cs.columbia.edu>,
	Linux IOMMU <iommu@...ts.linux-foundation.org>,
	VirtualOpenSystems Technical Team <tech@...tualopensystems.com>,
	KVM devel mailing list <kvm@...r.kernel.org>,
	Christoffer Dall <christoffer.dall@...aro.org>,
	Will Deacon <will.deacon@....com>,
	Kim Phillips <kim.phillips@...escale.com>,
	Eric Auger <eric.auger@...aro.org>,
	Marc Zyngier <marc.zyngier@....com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'

On Fri, Sep 26, 2014 at 5:37 PM, Russell King - ARM Linux
<linux@....linux.org.uk> wrote:
> On Tue, Sep 23, 2014 at 04:46:06PM +0200, Antonios Motakis wrote:
>> As already demonstrated with PCI [1] and the platform bus [2], a
>> driver_override property in sysfs can be used to bypass the id matching
>> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
>> device requested by the user.
>>
>> [1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
>> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>>
>> Signed-off-by: Antonios Motakis <a.motakis@...tualopensystems.com>
>
> I have to ask why this is even needed in the first place.  To take the
> example in [2], what's wrong with:
>
> echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
> echo fff51000.ethernet > /sys/bus/platform/drivers/vfio-platform/bind
>
> and similar for AMBA.
>
> All we would need to do is to introduce a way of having a driver accept
> explicit bind requests.

I don't have strong feelings on whether it should be done one way or
the other, however the approach proposed here is identical to the one
already accepted in mainline for PCI and platform devices. Should we
do something different for AMBA?

>
> In any case:
>
>> +static ssize_t driver_override_store(struct device *_dev,
>> +                                  struct device_attribute *attr,
>> +                                  const char *buf, size_t count)
>> +{
>> +     struct amba_device *dev = to_amba_device(_dev);
>> +     char *driver_override, *old = dev->driver_override, *cp;
>> +
>> +     if (count > PATH_MAX)
>> +             return -EINVAL;
>> +
>> +     driver_override = kstrndup(buf, count, GFP_KERNEL);
>> +     if (!driver_override)
>> +             return -ENOMEM;
>> +
>> +     cp = strchr(driver_override, '\n');
>> +     if (cp)
>> +             *cp = '\0';
>
> I hope that is not replicated everywhere.  This allows up to a page to be
> allocated, even when the first byte may be a newline.  This is wasteful.
>
> How about:
>
>         if (count > PATH_MAX)
>                 return -EINVAL;
>
>         cp = strnchr(buf, count, '\n');
>         if (cp)
>                 count = cp - buf - 1;
>
>         if (count) {
>                 driver_override = kstrndup(buf, count, GFP_KERNEL);
>                 if (!driver_override)
>                         return -ENOMEM;
>         } else {
>                 driver_override = NULL;
>         }
>
>         kfree(dev->driver_override);
>         dev->driver_override = driver_override;

Ack.

>
> Also:
>
>> +static ssize_t driver_override_show(struct device *_dev,
>> +                                 struct device_attribute *attr, char *buf)
>> +{
>> +     struct amba_device *dev = to_amba_device(_dev);
>> +
>> +     return sprintf(buf, "%s\n", dev->driver_override);
>> +}
>
> Do we really want to do a NULL pointer dereference here?

I'll add a check here.

Thanks

-- 
Antonios Motakis
Virtual Open Systems
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/