linux-kernel - Re: Possible race in dev_coredumpm()-del

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: <87pmlkdk6i.ffs@tglx>
Date:   Thu, 14 Apr 2022 12:38:13 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Mukesh Ojha <quic_mojha@...cinc.com>
Cc:     linux-kernel@...r.kernel.org, sboyd@...nel.org,
        johannes@...solutions.net, rafael@...nel.org
Subject: Re: Possible race in dev_coredumpm()-del_timer() path

On Wed, Apr 13 2022 at 12:58, Greg KH wrote:
> On Wed, Apr 13, 2022 at 03:46:39PM +0530, Mukesh Ojha wrote:
>> 	p1 					p2(X)
>> 
>>    dev_coredump() uevent sent to userspace
>>    device_add()  =========================> userspace process X reads the uevents
>>                                             writes to devcd fd which
>>                                             results into writes to
>> 
>>                                             devcd_data_write()
>> 					      mod_delayed_work()
>>                                                 try_to_grab_pending()
>> 						  del_timer()
>> 						   debug_assert_init()
>>   INIT_DELAYED_WORK 
>>   schedule_delayed_work
>> 						    debug_object_fixup()
>
> Why do you have object debugging enabled?  That's going to take a LONG
> time, and will find bugs in your code.  Perhaps like this one?

It's not finding bugs in his code. It finds bug in the upstream
dev_coredump code.

> And if you turn object debugging off, what happens?

The debugobject splat goes away, but the problem persists.

    device_add()        -> uevent

Preemption or concurrency:

    devcd_data_write()
       mod_delayed_work(..., w, 0);      <- Uninitialized.

The dev_coredump code exposes the device before it is fully initialized
and a write ending up in devcd_data_write() touches uninitialized work.

It does not help to move the initialization before device_add() as that
creates another problem:

    INIT_DELAYED_WORK(w)
    ...
    device_add()        -> uevent

Preemption or concurrency:

    devcd_data_write()
      mod_delayed_work(..., w, 0);      <- Schedules work immediately

    work_queue_runs()
      devcd_del(w)
        device_del()
        put_device()                    <- Drops the last reference

    initialization continues...
    
So, yes this needs serialization of some sort.

Same problem vs. disabled_store().

Thanks,

        tglx