Am 29. Mai 2016 19:11:45 MESZ, schrieb Cameron Gutman <aicommander@xxxxxxxxx>:
>
>> On May 29, 2016, at 7:11 AM, Manuel Reimer
><mail+linux-input@xxxxxxxxxxx> wrote:
>>
>> Hello,
>>
>> I had a deeper look at the kernel panic, happening if there are
>rumble effects loaded and the USB plug is pulled.
>>
>> The reason for this is similar to the one, I fixed in uinput some
>days ago.
>>
>> In "sony_remove" the memory for "output_report_dmabuf" is freed.
>> Then, a few lines later, "hid_hw_stop" is called.
>> This now tries to cleanup and causes ff_memless to try to send out a
>new rumble event which should turn both motor speeds to zero.
>> To get this processed, "sc->send_output_report" is called, which, for
>example, ends up in "dualshock4_send_output_report".
>> This function will now use the, already freed,
>"output_report_dmabuf".
>>
>> My patch zeroes out "output_report_dmabuf" after freeing and checks
>for this in the "send_output_report" functions. There may be other ways
>to fix this, so please tell me if you prefer some other way.
>>
>> I've added a one-line comment above the memory pointer check, as, in
>my opinion, it is not obvious what is happening here.
>>
>> Signed-off-by: Manuel Reimer <mail@xxxxxxxxxxx>
>>
>> --- a/drivers/hid/hid-sony.c 2016-05-13 16:13:00.339346161 +0200
>> +++ b/drivers/hid/hid-sony.c 2016-05-29 13:54:25.452029787 +0200
>> @@ -1809,6 +1809,10 @@ static void sixaxis_send_output_report(s
>> (struct sixaxis_output_report *)sc->output_report_dmabuf;
>> int n;
>>
>> + /* If called via hid_hw_stop, then our memory is already gone! */
>> + if (!report)
>> + return;
>> +
>> /* Initialize the report with default values */
>> memcpy(report, &default_report, sizeof(struct
>sixaxis_output_report));
>>
>> @@ -1853,6 +1857,10 @@ static void dualshock4_send_output_repor
>> __u8 *buf = sc->output_report_dmabuf;
>> int offset;
>>
>> + /* If called via hid_hw_stop, then our memory is already gone! */
>> + if (!buf)
>> + return;
>> +
>> if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
>> memset(buf, 0, DS4_REPORT_0x05_SIZE);
>> buf[0] = 0x05;
>> @@ -1899,6 +1907,10 @@ static void motion_send_output_report(st
>> struct motion_output_report_02 *report =
>> (struct motion_output_report_02 *)sc->output_report_dmabuf;
>>
>> + /* If called via hid_hw_stop, then our memory is already gone! */
>> + if (!report)
>> + return;
>> +
>> memset(report, 0, MOTION_REPORT_0x02_SIZE);
>>
>> report->type = 0x02; /* set leds */
>> @@ -2426,6 +2438,7 @@ static void sony_remove(struct hid_devic
>> sony_cancel_work_sync(sc);
>>
>> kfree(sc->output_report_dmabuf);
>> + sc->output_report_dmabuf = NULL;
>
>What prevents one of the send_output_report() functions from accessing
>sc->output_report_dmabuf after it’s been passed to kfree() but before
>you’ve set it to NULL? Why not simply wait to free output_report_dmabuf
>until after hid_hw_stop returns?
Good point! But I'm unsure how these workqueues work in kernel. Isn't it possible that send_output_report is called after sony_remove already finished executing (still a task in queue) and so the already freed memory is accessed, again?
>
>>
>> sony_remove_dev_list(sc);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>linux-input" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
