From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D16C8C433FE for ; Mon, 6 Dec 2021 21:48:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1352997AbhLFVvz (ORCPT ); Mon, 6 Dec 2021 16:51:55 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39242 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1352779AbhLFVvw (ORCPT ); Mon, 6 Dec 2021 16:51:52 -0500 Received: from mail-ed1-x530.google.com (mail-ed1-x530.google.com [IPv6:2a00:1450:4864:20::530]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 934A4C0613F8 for ; Mon, 6 Dec 2021 13:48:23 -0800 (PST) Received: by mail-ed1-x530.google.com with SMTP id x6so48552021edr.5 for ; Mon, 06 Dec 2021 13:48:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=6Eip2d/hVRGSLRlvh3UtzxqF3/zCUzzqztI5oHT+KdI=; b=EvMTxmIsfoaSzvFvf70bWgCu1ARC+fwDYdQj+lqJZ4N07cj76CSqLJjaGNO12txDXp j6FhDV5fNm0g3TyHS8kLuH03PC5FU4DT0JuLNC1qxdJuQnoGrNKGDEDPhHLKwutb/RJm bgB4KLujTRxMH5a9X/em6adrlxpsOgxMiornk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6Eip2d/hVRGSLRlvh3UtzxqF3/zCUzzqztI5oHT+KdI=; b=gSLbv8rnC+cmibk3dT1ZaiYrZ/l20wpcNWug5vlXBHpEFZrzfc8QqgayIFNgHil5Ev QIqxIHNjXruTNovH/ACQSbKHAGssSfii0QvkO+6zSEvX4wTaO24F2qSklNVthobR4skw 06/M2OSjDx9clRQaxCIzUdsbyfCH2aitkwezrc+Vd9SeoGnt9lX+B12h6MxjuKLaZ+oT EGaqE6yoajvZuACj0e2tXhbjEByqAVUW5Xz3Qt7RcXLK1y1NUrY6p82gVMlWBYTYID+d cyMhRtLd7LvQDSrDZ5do5WTDc6mCI71TBDarTYXf49cpVFCs84n5K3X+FrXzTdfg//Ta xQig== X-Gm-Message-State: AOAM531o+gfPuzFcMRBK21FNevu/GAFNJbJk8wOhkYrmdxmA8f9nLIWX gIUhS4O9WL8ovuTW4aE7J5JHILsbCrOJr1nT X-Google-Smtp-Source: ABdhPJwUlF3RnUVhhQd9dUq9gvwd73d1FKArjhLqxu03nlTIms5takbZhT8Z3wUfnY0UOlDMb0nKPg== X-Received: by 2002:a05:6402:d0b:: with SMTP id eb11mr2586624edb.67.1638827301530; Mon, 06 Dec 2021 13:48:21 -0800 (PST) Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com. [209.85.221.47]) by smtp.gmail.com with ESMTPSA id hx14sm7128484ejc.92.2021.12.06.13.48.20 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Dec 2021 13:48:20 -0800 (PST) Received: by mail-wr1-f47.google.com with SMTP id a18so25298857wrn.6 for ; Mon, 06 Dec 2021 13:48:20 -0800 (PST) X-Received: by 2002:adf:9d88:: with SMTP id p8mr48026383wre.140.1638827300398; Mon, 06 Dec 2021 13:48:20 -0800 (PST) MIME-Version: 1.0 References: <20211204002301.116139-1-ebiggers@kernel.org> <20211204002301.116139-3-ebiggers@kernel.org> In-Reply-To: From: Linus Torvalds Date: Mon, 6 Dec 2021 13:48:04 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/2] aio: fix use-after-free due to missing POLLFREE handling To: Eric Biggers Cc: Alexander Viro , Benjamin LaHaise , linux-aio@kvack.org, linux-fsdevel , Linux Kernel Mailing List , Ramji Jiyani , Christoph Hellwig , Oleg Nesterov , Jens Axboe , stable Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Dec 6, 2021 at 11:54 AM Eric Biggers wrote: > > It could be fixed by converting signalfd and binder to use something like this, > right? > > #define wake_up_pollfree(x) \ > __wake_up(x, TASK_NORMAL, 0, poll_to_key(EPOLLHUP | POLLFREE)) Yeah, that looks better to me. That said, maybe it would be even better to then make it more explicit about what it does, and not make it look like just another wakeup with an odd parameter. IOW, maybe that "pollfree()" function itself could very much do the waitqueue entry removal on each entry using list_del_init(), and not expect the wakeup code for the entry to do so. I think that kind of explicit "this removes all entries from the wait list head" is better than "let's do a wakeup and expect all entries to magically implicitly remove themselves". After all, that "implicitly remove themselves" was what didn't happen, and caused the bug in the first place. And all the normal cases, that don't care about POLLFREE at all, because their waitqueues don't go away from under them, wouldn't care, because "list_del_init()" still leaves a valid self-pointing list in place, so if they do list_del() afterwards, nothing happens. I dunno. But yes, that wake_up_pollfree() of yours certainly looks better than what we have now. > As for eliminating POLLFREE entirely, that would require that the waitqueue > heads be moved to a location which has a longer lifetime. Yeah, the problem with aio and epoll is exactly that they end up using waitqueue heads without knowing what they are. I'm not at all convinced that there aren't other situations where the thing the waitqueue head is embedded might not have other lifetimes. The *common* situation is obviously that it's associated with a file, and the file pointer ends up holding the reference to whatever device or something (global list in a loadable module, or whatever) it is. Hmm. The poll_wait() callback function actually does get the 'struct file *' that the wait is associated with. I wonder if epoll queueing could actually increment the file ref when it creates its own wait entry, and release it at ep_remove_wait_queue()? Maybe epoll could avoid having to remove entries entirely that way - simply by virtue of having a ref to the files - and remove the need for having the ->whead pointer entirely (and remove the need for POLLFREE handling)? And maybe the signalfd case can do the same - instead of expecting exit() to clean up the list when sighand->count goes to zero, maybe the signalfd filp can just hold a ref to that 'struct sighand_struct', and it gets free'd whenever there are no signalfd users left? That would involve making signalfd_ctx actually tied to one particular 'struct signal', but that might be the right thing to do regardless (instead of making it always act on 'current' like it does now). So maybe with some re-organization, we could get rid of the need for POLLFREE entirely.. Anybody? But your patches are certainly simpler in that they just fix the status quo. Linus