From patchwork Mon Sep 30 03:06:06 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Koichiro Den X-Patchwork-Id: 1990722 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=185.125.189.65; helo=lists.ubuntu.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=patchwork.ozlabs.org) Received: from lists.ubuntu.com (lists.ubuntu.com [185.125.189.65]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4XH5Zj2RHhz1xsq for ; Mon, 30 Sep 2024 13:06:53 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=lists.ubuntu.com) by lists.ubuntu.com with esmtp (Exim 4.86_2) (envelope-from ) id 1sv6kH-0000I2-2o; Mon, 30 Sep 2024 03:06:45 +0000 Received: from smtp-relay-internal-1.internal ([10.131.114.114] helo=smtp-relay-internal-1.canonical.com) by lists.ubuntu.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1sv6kE-0000Gz-Rb for kernel-team@lists.ubuntu.com; Mon, 30 Sep 2024 03:06:42 +0000 Received: from mail-pj1-f70.google.com (mail-pj1-f70.google.com [209.85.216.70]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-1.canonical.com (Postfix) with ESMTPS id C0E7F3F2D4 for ; Mon, 30 Sep 2024 03:06:41 +0000 (UTC) Received: by mail-pj1-f70.google.com with SMTP id 98e67ed59e1d1-2e0c5819c57so2897094a91.2 for ; Sun, 29 Sep 2024 20:06:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727665600; x=1728270400; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=uL649up9eQ1wokc0YJLf+3qt92i3gsexCGGlwTnVEnE=; b=meYeoCqDLSYnESvybIXK9X/2rUrB95m9M4iaL0Z/wVS8qf9Ya9LyeKcd3uOlThXxzM NpnzoDIL4JO697VW05HaYdz2DqUofYIfjJKd1OvPfCKUX6clIV8c2wRJug5zuiEMiTWY dVQQniHzvcxkhs3JfjifmNyfJ7HO7F2RYmatuaYtkbwIHHCF0980LxD6r/jfkrQutiNR mOhQbv8YqtKSSb9yGLUHlMeeNmHZsiPC0AegX83sIWMAfZyyYGYj2Q7UG1ZSIkJP1duP z+DZRwqOLI2GYGG6Y299HRKAy4xHWs4LfJuym302jgwnJzs/wxVdmxNTFOaZWWWGZBQT uD/w== X-Gm-Message-State: AOJu0YzBB2QtFj3gnI7SltS8QNVhTEMCKV1J7rIQA5Frqndy2ylkZlmm EHH8+5kLrNtJqsAHd9OlkWj+m03c7xI6jwne914ohxn+ktB6ILYl41ZrGMzbUpNoqKiDXhBEt1E +6IH3lhA7D3OVAVTQM3o18eSg2EwQ0JjmfBKYRvzlErpvjosaJzUouJPvLk/UN5gy6U36UdEeT2 06w8F01eHPRw== X-Received: by 2002:a17:90a:ba96:b0:2e0:d1fb:9b2 with SMTP id 98e67ed59e1d1-2e0d1fb110amr10246633a91.33.1727665599925; Sun, 29 Sep 2024 20:06:39 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEjqJwg4FU+WRjq2ASgrZehxv+fStn/6rC0KrghoRWbtcFimC8dyXu2/hgCUPoL0ARfzsyqJg== X-Received: by 2002:a17:90a:ba96:b0:2e0:d1fb:9b2 with SMTP id 98e67ed59e1d1-2e0d1fb110amr10246599a91.33.1727665599243; Sun, 29 Sep 2024 20:06:39 -0700 (PDT) Received: from localhost.localdomain ([240f:74:7be:1:c835:dd5c:238f:3a73]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2e0daaf91e9sm5153092a91.52.2024.09.29.20.06.38 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 29 Sep 2024 20:06:38 -0700 (PDT) From: Koichiro Den To: kernel-team@lists.ubuntu.com Subject: [SRU][F][PATCH 1/1] btrfs: fix use-after-free after failure to create a snapshot Date: Mon, 30 Sep 2024 12:06:06 +0900 Message-ID: <20240930030610.591772-2-koichiro.den@canonical.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240930030610.591772-1-koichiro.den@canonical.com> References: <20240930030610.591772-1-koichiro.den@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Filipe Manana At ioctl.c:create_snapshot(), we allocate a pending snapshot structure and then attach it to the transaction's list of pending snapshots. After that we call btrfs_commit_transaction(), and if that returns an error we jump to 'fail' label, where we kfree() the pending snapshot structure. This can result in a later use-after-free of the pending snapshot: 1) We allocated the pending snapshot and added it to the transaction's list of pending snapshots; 2) We call btrfs_commit_transaction(), and it fails either at the first call to btrfs_run_delayed_refs() or btrfs_start_dirty_block_groups(). In both cases, we don't abort the transaction and we release our transaction handle. We jump to the 'fail' label and free the pending snapshot structure. We return with the pending snapshot still in the transaction's list; 3) Another task commits the transaction. This time there's no error at all, and then during the transaction commit it accesses a pointer to the pending snapshot structure that the snapshot creation task has already freed, resulting in a user-after-free. This issue could actually be detected by smatch, which produced the following warning: fs/btrfs/ioctl.c:843 create_snapshot() warn: '&pending_snapshot->list' not removed from list So fix this by not having the snapshot creation ioctl directly add the pending snapshot to the transaction's list. Instead add the pending snapshot to the transaction handle, and then at btrfs_commit_transaction() we add the snapshot to the list only when we can guarantee that any error returned after that point will result in a transaction abort, in which case the ioctl code can safely free the pending snapshot and no one can access it anymore. CC: stable@vger.kernel.org # 5.10+ Signed-off-by: Filipe Manana Signed-off-by: David Sterba (backported from commit 28b21c558a3753171097193b6f6602a94169093a) [koichiroden: Adjusted context due to missing commits: 9babda9f33fd ("btrfs: Remove async_transid from btrfs_mksubvol/create_subvol/create_snapshot") d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit")] CVE-2022-48733 Signed-off-by: Koichiro Den --- fs/btrfs/ioctl.c | 5 +---- fs/btrfs/transaction.c | 24 ++++++++++++++++++++++++ fs/btrfs/transaction.h | 2 ++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 674d774eb662..a061f8550e4c 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -864,10 +864,7 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, goto fail; } - spin_lock(&fs_info->trans_lock); - list_add(&pending_snapshot->list, - &trans->transaction->pending_snapshots); - spin_unlock(&fs_info->trans_lock); + trans->pending_snapshot = pending_snapshot; if (async_transid) { *async_transid = trans->transid; ret = btrfs_commit_transaction_async(trans, 1); diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 1d25bf0c55cc..b2cc5ea41e5d 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1947,6 +1947,27 @@ static inline void btrfs_wait_delalloc_flush(struct btrfs_trans_handle *trans) } } +/* + * Add a pending snapshot associated with the given transaction handle to the + * respective handle. This must be called after the transaction commit started + * and while holding fs_info->trans_lock. + * This serves to guarantee a caller of btrfs_commit_transaction() that it can + * safely free the pending snapshot pointer in case btrfs_commit_transaction() + * returns an error. + */ +static void add_pending_snapshot(struct btrfs_trans_handle *trans) +{ + struct btrfs_transaction *cur_trans = trans->transaction; + + if (!trans->pending_snapshot) + return; + + lockdep_assert_held(&trans->fs_info->trans_lock); + ASSERT(cur_trans->state >= TRANS_STATE_COMMIT_START); + + list_add(&trans->pending_snapshot->list, &cur_trans->pending_snapshots); +} + int btrfs_commit_transaction(struct btrfs_trans_handle *trans) { struct btrfs_fs_info *fs_info = trans->fs_info; @@ -2031,6 +2052,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) spin_lock(&fs_info->trans_lock); if (cur_trans->state >= TRANS_STATE_COMMIT_START) { + add_pending_snapshot(trans); + spin_unlock(&fs_info->trans_lock); refcount_inc(&cur_trans->use_count); ret = btrfs_end_transaction(trans); @@ -2105,6 +2128,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) * COMMIT_DOING so make sure to wait for num_writers to == 1 again. */ spin_lock(&fs_info->trans_lock); + add_pending_snapshot(trans); cur_trans->state = TRANS_STATE_COMMIT_DOING; spin_unlock(&fs_info->trans_lock); wait_event(cur_trans->writer_wait, diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h index cbede328bda5..8173fd9f9a6b 100644 --- a/fs/btrfs/transaction.h +++ b/fs/btrfs/transaction.h @@ -113,6 +113,8 @@ struct btrfs_trans_handle { struct btrfs_transaction *transaction; struct btrfs_block_rsv *block_rsv; struct btrfs_block_rsv *orig_rsv; + /* Set by a task that wants to create a snapshot. */ + struct btrfs_pending_snapshot *pending_snapshot; refcount_t use_count; unsigned int type; /*