Re: [PATCH v2] gpiolib-acpi: Duplicate con_id string when adding it to the crs lookup list

[Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>]

On Thu, Apr 28, 2016 at 5:30 AM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Thu, Apr 28, 2016 at 08:55:36PM +0900, Alexandre Courbot wrote:
>> On Thu, Apr 28, 2016 at 7:33 PM, Ville Syrjälä
>> <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>> > On Thu, Apr 28, 2016 at 02:04:10PM +0900, Alexandre Courbot wrote:
>> >> On Mon, Apr 25, 2016 at 10:01 PM,  <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>> >> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >> >
>> >> > Calling gpiod_get() from a module and then unloading the module leads to an
>> >> > oops due to acpi_can_fallback_to_crs() storing the pointer to the passed
>> >> > 'con_id' string onto acpi_crs_lookup_list. The next guy to come along will then
>> >> > try to access the string but the memory may now be gone with the module.
>> >> > Make a copy of the passed string instead, and store the copy on the list.
>> >> >
>> >> > BUG: unable to handle kernel paging request at ffffffffa03e7855
>> >> > IP: [<ffffffff81338322>] strcmp+0x12/0x30
>> >> > PGD 2a07067 PUD 2a08063 PMD 74720067 PTE 0
>> >> > Oops: 0000 [#1] PREEMPT SMP
>> >> > Modules linked in: i915(+) drm_kms_helper drm intel_gtt snd_hda_codec snd_hda_core i2c_algo_bit syscopya
>> >> > rea sysfillrect sysimgblt fb_sys_fops agpgart snd_soc_sst_bytcr_rt5640 coretemp hwmon intel_rapl intel_soc_dts_thermal
>> >> > punit_atom_debug snd_soc_rt5640 snd_soc_rl6231 serio snd_intel_sst_acpi snd_intel_sst_core video snd_soc_sst_mfld_platf
>> >> > orm snd_soc_sst_match backlight int3402_thermal processor_thermal_device int3403_thermal int3400_thermal acpi_thermal_r
>> >> > el snd_soc_core intel_soc_dts_iosf int340x_thermal_zone snd_compress i2c_hid hid snd_pcm snd_timer snd soundcore evdev
>> >> > sch_fq_codel efivarfs ipv6 autofs4 [last unloaded: drm]
>> >> > CPU: 2 PID: 3064 Comm: modprobe Tainted: G     U  W       4.6.0-rc3-ffrd-ipvr+ #302
>> >> > Hardware name: Intel Corp. VALLEYVIEW C0 PLATFORM/BYT-T FFD8, BIOS BLAKFF81.X64.0088.R10.1403240443 FFD8
>> >> > _X64_R_2014_13_1_00 03/24/2014
>> >> > task: ffff8800701cd200 ti: ffff880070034000 task.ti: ffff880070034000
>> >> > RIP: 0010:[<ffffffff81338322>]  [<ffffffff81338322>] strcmp+0x12/0x30
>> >> > RSP: 0000:ffff880070037748  EFLAGS: 00010286
>> >> > RAX: 0000000080000000 RBX: ffff88007a342800 RCX: 0000000000000006
>> >> > RDX: 0000000000000006 RSI: ffffffffa054f856 RDI: ffffffffa03e7856
>> >> > RBP: ffff880070037748 R08: 0000000000000000 R09: 0000000000000001
>> >> > R10: 0000000000000000 R11: 0000000000000000 R12: ffffffffa054f855
>> >> > R13: ffff88007281cae0 R14: 0000000000000010 R15: ffffffffffffffea
>> >> > FS:  00007faa51447700(0000) GS:ffff880079300000(0000) knlGS:0000000000000000
>> >> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> >> > CR2: ffffffffa03e7855 CR3: 0000000041eba000 CR4: 00000000001006e0
>> >> > Stack:
>> >> >  ffff880070037770 ffffffff8136ad28 ffffffffa054f855 0000000000000000
>> >> >  ffff88007a0a2098 ffff8800700377e8 ffffffff8136852e ffff88007a342800
>> >> >  00000007700377a0 ffff8800700377a0 ffffffff81412442 70672d6c656e6170
>> >> > Call Trace:
>> >> >  [<ffffffff8136ad28>] acpi_can_fallback_to_crs+0x88/0x100
>> >> >  [<ffffffff8136852e>] gpiod_get_index+0x25e/0x310
>> >> >  [<ffffffff81412442>] ? mipi_dsi_attach+0x22/0x30
>> >> >  [<ffffffff813685f2>] gpiod_get+0x12/0x20
>> >> >  [<ffffffffa04fcf41>] intel_dsi_init+0x421/0x480 [i915]
>> >> >  [<ffffffffa04d3783>] intel_modeset_init+0x853/0x16b0 [i915]
>> >> >  [<ffffffffa0504864>] ? intel_setup_gmbus+0x214/0x260 [i915]
>> >> >  [<ffffffffa0510158>] i915_driver_load+0xdc8/0x19b0 [i915]
>> >> >  [<ffffffff8160fb53>] ? _raw_spin_unlock_irqrestore+0x43/0x70
>> >> >  [<ffffffffa026b13b>] drm_dev_register+0xab/0xc0 [drm]
>> >> >  [<ffffffffa026d7b3>] drm_get_pci_dev+0x93/0x1f0 [drm]
>> >> >  [<ffffffff8160fb53>] ? _raw_spin_unlock_irqrestore+0x43/0x70
>> >> >  [<ffffffffa043f1f4>] i915_pci_probe+0x34/0x50 [i915]
>> >> >  [<ffffffff81379751>] pci_device_probe+0x91/0x100
>> >> >  [<ffffffff8141a75a>] driver_probe_device+0x20a/0x2d0
>> >> >  [<ffffffff8141a8be>] __driver_attach+0x9e/0xb0
>> >> >  [<ffffffff8141a820>] ? driver_probe_device+0x2d0/0x2d0
>> >> >  [<ffffffff81418439>] bus_for_each_dev+0x69/0xa0
>> >> >  [<ffffffff8141a04e>] driver_attach+0x1e/0x20
>> >> >  [<ffffffff81419c20>] bus_add_driver+0x1c0/0x240
>> >> >  [<ffffffff8141b6d0>] driver_register+0x60/0xe0
>> >> >  [<ffffffff81377d20>] __pci_register_driver+0x60/0x70
>> >> >  [<ffffffffa026d9f4>] drm_pci_init+0xe4/0x110 [drm]
>> >> >  [<ffffffff810ce04e>] ? trace_hardirqs_on+0xe/0x10
>> >> >  [<ffffffffa02f1000>] ? 0xffffffffa02f1000
>> >> >  [<ffffffffa02f1094>] i915_init+0x94/0x9b [i915]
>> >> >  [<ffffffff810003bb>] do_one_initcall+0x8b/0x1c0
>> >> >  [<ffffffff810eb616>] ? rcu_read_lock_sched_held+0x86/0x90
>> >> >  [<ffffffff811de6d6>] ? kmem_cache_alloc_trace+0x1f6/0x270
>> >> >  [<ffffffff81183826>] do_init_module+0x60/0x1dc
>> >> >  [<ffffffff81115a8d>] load_module+0x1d0d/0x2390
>> >> >  [<ffffffff811120b0>] ? __symbol_put+0x70/0x70
>> >> >  [<ffffffff811f41b2>] ? kernel_read_file+0x92/0x120
>> >> >  [<ffffffff811162f4>] SYSC_finit_module+0xa4/0xb0
>> >> >  [<ffffffff8111631e>] SyS_finit_module+0xe/0x10
>> >> >  [<ffffffff81001ff3>] do_syscall_64+0x63/0x350
>> >> >  [<ffffffff816103da>] entry_SYSCALL64_slow_path+0x25/0x25
>> >> > Code: f7 48 8d 76 01 48 8d 52 01 0f b6 4e ff 84 c9 88 4a ff 75 ed 5d c3 0f 1f 00 55 48 89 e5 eb 04 84 c0
>> >> >  74 18 48 8d 7f 01 48 8d 76 01 <0f> b6 47 ff 3a 46 ff 74 eb 19 c0 83 c8 01 5d c3 31 c0 5d c3 66
>> >> > RIP  [<ffffffff81338322>] strcmp+0x12/0x30
>> >> >  RSP <ffff880070037748>
>> >> > CR2: ffffffffa03e7855
>> >> >
>> >> > v2: Make the copied con_id const
>> >> >
>> >> > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>> >> > Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> >> > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> >> > Cc: Alexandre Courbot <gnurou@xxxxxxxxx>
>> >> > Cc: stable@xxxxxxxxxxxxxxx
>> >> > Fixes: 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio lookups")
>> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >> > ---
>> >> >  drivers/gpio/gpiolib-acpi.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
>> >> > index 682070d20f00..2dc52585e3f2 100644
>> >> > --- a/drivers/gpio/gpiolib-acpi.c
>> >> > +++ b/drivers/gpio/gpiolib-acpi.c
>> >> > @@ -977,7 +977,7 @@ bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
>> >> >                 lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
>> >> >                 if (lookup) {
>> >> >                         lookup->adev = adev;
>> >> > -                       lookup->con_id = con_id;
>> >> > +                       lookup->con_id = kstrdup(con_id, GFP_KERNEL);
>> >>
>> >> Agree with the idea, but where is this newly-allocated memory freed?
>> >
>> > It isn't. Neither is the 'lookup' thing itself.
>>
>> Are we ok with that? Leaking memory is slightly better than crashing a
>> driver, but that is still not something to be fond of...
>
> When would you free it?

It is not supposed to be freed. We need to note that someone at some
point requested gpio with given name, so if someone else ever requests
gpio with different name we shoudl not be falling back to _CRS.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html