linux-kernel - Re: drivers/firmware/google/vpd.c: duplicate sysfs file

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: <CABXOdTfc1wWmjrExpGkpx6ccoiKBadVLCdSexBXqNCfG7apd=w@mail.gmail.com>
Date:   Mon, 13 Nov 2017 14:24:23 -0800
From:   Guenter Roeck <groeck@...gle.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Guenter Roeck <groeck@...omium.org>,
        Wei-Ning Huang <wnhuang@...gle.com>
Subject: Re: drivers/firmware/google/vpd.c: duplicate sysfs file

On Mon, Nov 13, 2017 at 2:01 PM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> On Mon, Nov 13, 2017 at 12:14:41PM -0800, Guenter Roeck wrote:
>> The sysfs warning, yes. However, after unbinding and rebinding the
>> driver, "cat /sys/firmware/vpd/rw_raw" will result in a crash.
>>
>> Sequence:
>>
>> echo vpd > /sys/bus/platform/drivers/vpd/unbind
>> echo vpd > /sys/bus/platform/drivers/vpd/bind # <-- nasty message
>>
>> cat /sys/firmware/vpd/rw_raw # <-- crash
>
> I'd disable unbind (.suppress_bind_attrs = true) and commented out
> unload for now. And then looked into fixing properly.
>

Fixing properly is easy as long as we retain the platform device; I
already have a set of patches doing just that. I have no idea how to
implement the non-platform-device variant, if that is what you mean
with "properly".

Guenter

>>
>> Guenter
>>
>> On Mon, Nov 13, 2017 at 11:23 AM, Greg Kroah-Hartman
>> <gregkh@...uxfoundation.org> wrote:
>> > On Mon, Nov 13, 2017 at 11:09:21AM -0800, Guenter Roeck wrote:
>> >> On Mon, Nov 13, 2017 at 11:04 AM, Dmitry Torokhov
>> >> <dmitry.torokhov@...il.com> wrote:
>> >> >
>> >> > On Mon, Nov 13, 2017 at 10:18:35AM -0800, Randy Dunlap wrote:
>> >> > > On 11/13/2017 06:41 AM, Guenter Roeck wrote:
>> >> > > >
>> >> > > >
>> >> > > > On Sun, Nov 12, 2017 at 10:36 PM, Randy Dunlap <rdunlap@...radead.org <mailto:rdunlap@...radead.org>> wrote:
>> >> > > >
>> >> > > >     sysfs: cannot create duplicate filename '/devices/platform/vpd'
>> >> > > >
>> >> > > >     on the second load of this driver.  I.e.,
>> >> > > >
>> >> > > >     modprobe vpd-sysfs
>> >> > > >     rmmod vpd-sysfs
>> >> > > >     modprobe vpd-sysfs
>> >> > > >     [boom]
>> >> > > >
>> >> > > > Neither the platform device nor the platform driver driver are ever unregistered, so this isn't entirely surprising. I'll try to reproduce and send a patch.
>> >> > >
>> >> > >
>> >> > > Seems to be a common theme:
>> >> > >
>> >> > > google> grep --color=never "platform.*register" *.c
>> >> > > coreboot_table-acpi.c:        return platform_driver_register(&coreboot_table_acpi_driver);
>> >> > > coreboot_table-of.c:  return platform_driver_register(&coreboot_table_of_driver);
>> >> >
>> >> > These are not unloadable (for better or worse) - they do not have
>> >> > module_exit() in them.
>> >> >
>> >> > >
>> >> > > gsmi.c:       gsmi_dev.pdev = platform_device_register_full(&gsmi_dev_info);
>> >> > > gsmi.c:       platform_device_unregister(gsmi_dev.pdev);
>> >> > > gsmi.c:       platform_device_unregister(gsmi_dev.pdev);
>> >> > > [looks good]
>> >> > >
>> >> > > memconsole-coreboot.c:        pdev = platform_device_register_simple("memconsole", -1, NULL, 0);
>> >> > > memconsole-coreboot.c:        platform_driver_register(&memconsole_driver);
>> >> >
>> >> > Same here: not unloadable.
>> >> >
>> >> > >
>> >> > > vpd.c:        pdev = platform_device_register_simple("vpd", -1, NULL, 0);
>> >> > > vpd.c:        platform_driver_register(&vpd_driver);
>> >> >
>> >> > Arguably this should not even be a platform driver, there is no hardware
>> >> > behind it. I was planning on purring some notifiers into coreboot table
>> >> > driver and using notifiers to attach vpd to them. -ENOTIME though.
>> >> >
>> >> Two options for now: clean it up and make it unloadable, or make it bool
>> >> and drop the exit function. Any preference ?
>> >>
>> >> The problem is easy to reproduce even with the driver is built into
>> >> the kernel with a simple unbind/bind sequence. And after the unbind,
>> >> it is easy to crash the system since the sysfs attributes are still there.
>> >
>> > The kernel should not 'crash', just spit out a nasty warning, right?
>> >
>> > thanks,
>> >
>> > greg k-h
>
> --
> Dmitry