linux-kernel - Re: [PATCH 1/4] fsnotify: fix pinning of marks and groups

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: <CAOssrKcGffGCrMU9xhNQwFdNwj0h=Rzb_Ww_z_Db0xh1LYJC1g@mail.gmail.com>
Date:   Fri, 20 Oct 2017 13:56:29 +0200
From:   Miklos Szeredi <mszeredi@...hat.com>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Jan Kara <jack@...e.cz>, Xiong Zhou <xzhou@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] fsnotify: fix pinning of marks and groups

On Fri, Oct 20, 2017 at 1:37 PM, Amir Goldstein <amir73il@...il.com> wrote:
> On Thu, Oct 19, 2017 at 4:58 PM, Miklos Szeredi <mszeredi@...hat.com> wrote:
>> We may fail to pin one of the marks in fsnotify_prepare_user_wait() when
>> dropping the srcu read lock, resulting in use after free at the next
>> iteration.
>>
>> Solution is to store both marks in iter_info instead of just the one we'll
>> be sending the event for, as well as pinning the group for both (if they
>> have the same group: fine, pinnig it twice doesn't hurt).

I was thinking about this one and actually pinning only the single
group for the used mark was correct, since the other, unused mark
won't have its group dereferenced while pinned.

Only there doesn't appear to be a way to make this work without adding
complexity :(


>>
>> Also blind increment of group's user_waits is not enough, we could be far
>> enough in the group's destruction that it isn't taken into account.
>> Instead we need to check (under lock) that the mark is still attached to
>> the group after having obtained a ref to the mark.  If not, skip it.
>>
>> In the process of fixing the above, clean up fsnotify_prepare_user_wait()/
>> fsnotify_finish_user_wait().
>>
>
> Change looks correct, but it was so hard to follow this patch.
> The moving of helpers around created a completely garbled diff
> where it is impossible to see the logic changed.
>
> Suggest to separate to 3 patches: create helpers (move them around),
> pass the 2 marks in iter_info and fix increment of group's user_waits.

Yeah, I'll do that.

Thanks,
Miklos