mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Ben Hutchings <ben@decadent.org.uk>
To: linux-kernel@vger.kernel.org, stable@vger.kernel.org
Cc: akpm@linux-foundation.org, "Filipe Manana" <fdmanana@suse.com>
Subject: [PATCH 3.2 15/52] Btrfs: fix truncation of compressed and inlined extents
Date: Tue, 24 Nov 2015 22:33:59 +0000	[thread overview]
Message-ID: <lsq.1448404439.200660671@decadent.org.uk> (raw)
In-Reply-To: <lsq.1448404438.351251660@decadent.org.uk>

3.2.74-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Filipe Manana <fdmanana@suse.com>

commit 0305cd5f7fca85dae392b9ba85b116896eb7c1c7 upstream.

When truncating a file to a smaller size which consists of an inline
extent that is compressed, we did not discard (or made unusable) the
data between the new file size and the old file size, wasting metadata
space and allowing for the truncated data to be leaked and the data
corruption/loss mentioned below.
We were also not correctly decrementing the number of bytes used by the
inode, we were setting it to zero, giving a wrong report for callers of
the stat(2) syscall. The fsck tool also reported an error about a mismatch
between the nbytes of the file versus the real space used by the file.

Now because we weren't discarding the truncated region of the file, it
was possible for a caller of the clone ioctl to actually read the data
that was truncated, allowing for a security breach without requiring root
access to the system, using only standard filesystem operations. The
scenario is the following:

   1) User A creates a file which consists of an inline and compressed
      extent with a size of 2000 bytes - the file is not accessible to
      any other users (no read, write or execution permission for anyone
      else);

   2) The user truncates the file to a size of 1000 bytes;

   3) User A makes the file world readable;

   4) User B creates a file consisting of an inline extent of 2000 bytes;

   5) User B issues a clone operation from user A's file into its own
      file (using a length argument of 0, clone the whole range);

   6) User B now gets to see the 1000 bytes that user A truncated from
      its file before it made its file world readbale. User B also lost
      the bytes in the range [1000, 2000[ bytes from its own file, but
      that might be ok if his/her intention was reading stale data from
      user A that was never supposed to be public.

Note that this contrasts with the case where we truncate a file from 2000
bytes to 1000 bytes and then truncate it back from 1000 to 2000 bytes. In
this case reading any byte from the range [1000, 2000[ will return a value
of 0x00, instead of the original data.

This problem exists since the clone ioctl was added and happens both with
and without my recent data loss and file corruption fixes for the clone
ioctl (patch "Btrfs: fix file corruption and data loss after cloning
inline extents").

So fix this by truncating the compressed inline extents as we do for the
non-compressed case, which involves decompressing, if the data isn't already
in the page cache, compressing the truncated version of the extent, writing
the compressed content into the inline extent and then truncate it.

The following test case for fstests reproduces the problem. In order for
the test to pass both this fix and my previous fix for the clone ioctl
that forbids cloning a smaller inline extent into a larger one,
which is titled "Btrfs: fix file corruption and data loss after cloning
inline extents", are needed. Without that other fix the test fails in a
different way that does not leak the truncated data, instead part of
destination file gets replaced with zeroes (because the destination file
has a larger inline extent than the source).

  seq=`basename $0`
  seqres=$RESULT_DIR/$seq
  echo "QA output created by $seq"
  tmp=/tmp/$$
  status=1	# failure is the default!
  trap "_cleanup; exit \$status" 0 1 2 3 15

  _cleanup()
  {
      rm -f $tmp.*
  }

  # get standard environment, filters and checks
  . ./common/rc
  . ./common/filter

  # real QA test starts here
  _need_to_be_root
  _supported_fs btrfs
  _supported_os Linux
  _require_scratch
  _require_cloner

  rm -f $seqres.full

  _scratch_mkfs >>$seqres.full 2>&1
  _scratch_mount "-o compress"

  # Create our test files. File foo is going to be the source of a clone operation
  # and consists of a single inline extent with an uncompressed size of 512 bytes,
  # while file bar consists of a single inline extent with an uncompressed size of
  # 256 bytes. For our test's purpose, it's important that file bar has an inline
  # extent with a size smaller than foo's inline extent.
  $XFS_IO_PROG -f -c "pwrite -S 0xa1 0 128"   \
          -c "pwrite -S 0x2a 128 384" \
          $SCRATCH_MNT/foo | _filter_xfs_io
  $XFS_IO_PROG -f -c "pwrite -S 0xbb 0 256" $SCRATCH_MNT/bar | _filter_xfs_io

  # Now durably persist all metadata and data. We do this to make sure that we get
  # on disk an inline extent with a size of 512 bytes for file foo.
  sync

  # Now truncate our file foo to a smaller size. Because it consists of a
  # compressed and inline extent, btrfs did not shrink the inline extent to the
  # new size (if the extent was not compressed, btrfs would shrink it to 128
  # bytes), it only updates the inode's i_size to 128 bytes.
  $XFS_IO_PROG -c "truncate 128" $SCRATCH_MNT/foo

  # Now clone foo's inline extent into bar.
  # This clone operation should fail with errno EOPNOTSUPP because the source
  # file consists only of an inline extent and the file's size is smaller than
  # the inline extent of the destination (128 bytes < 256 bytes). However the
  # clone ioctl was not prepared to deal with a file that has a size smaller
  # than the size of its inline extent (something that happens only for compressed
  # inline extents), resulting in copying the full inline extent from the source
  # file into the destination file.
  #
  # Note that btrfs' clone operation for inline extents consists of removing the
  # inline extent from the destination inode and copy the inline extent from the
  # source inode into the destination inode, meaning that if the destination
  # inode's inline extent is larger (N bytes) than the source inode's inline
  # extent (M bytes), some bytes (N - M bytes) will be lost from the destination
  # file. Btrfs could copy the source inline extent's data into the destination's
  # inline extent so that we would not lose any data, but that's currently not
  # done due to the complexity that would be needed to deal with such cases
  # (specially when one or both extents are compressed), returning EOPNOTSUPP, as
  # it's normally not a very common case to clone very small files (only case
  # where we get inline extents) and copying inline extents does not save any
  # space (unlike for normal, non-inlined extents).
  $CLONER_PROG -s 0 -d 0 -l 0 $SCRATCH_MNT/foo $SCRATCH_MNT/bar

  # Now because the above clone operation used to succeed, and due to foo's inline
  # extent not being shinked by the truncate operation, our file bar got the whole
  # inline extent copied from foo, making us lose the last 128 bytes from bar
  # which got replaced by the bytes in range [128, 256[ from foo before foo was
  # truncated - in other words, data loss from bar and being able to read old and
  # stale data from foo that should not be possible to read anymore through normal
  # filesystem operations. Contrast with the case where we truncate a file from a
  # size N to a smaller size M, truncate it back to size N and then read the range
  # [M, N[, we should always get the value 0x00 for all the bytes in that range.

  # We expected the clone operation to fail with errno EOPNOTSUPP and therefore
  # not modify our file's bar data/metadata. So its content should be 256 bytes
  # long with all bytes having the value 0xbb.
  #
  # Without the btrfs bug fix, the clone operation succeeded and resulted in
  # leaking truncated data from foo, the bytes that belonged to its range
  # [128, 256[, and losing data from bar in that same range. So reading the
  # file gave us the following content:
  #
  # 0000000 a1 a1 a1 a1 a1 a1 a1 a1 a1 a1 a1 a1 a1 a1 a1 a1
  # *
  # 0000200 2a 2a 2a 2a 2a 2a 2a 2a 2a 2a 2a 2a 2a 2a 2a 2a
  # *
  # 0000400
  echo "File bar's content after the clone operation:"
  od -t x1 $SCRATCH_MNT/bar

  # Also because the foo's inline extent was not shrunk by the truncate
  # operation, btrfs' fsck, which is run by the fstests framework everytime a
  # test completes, failed reporting the following error:
  #
  #  root 5 inode 257 errors 400, nbytes wrong

  status=0
  exit

Signed-off-by: Filipe Manana <fdmanana@suse.com>
[bwh: Backported to 3.2:
 - Adjust parameters to btrfs_truncate_page() and btrfs_truncate_item()
 - Pass transaction pointer into truncate_inline_extent()
 - Add prototype of btrfs_truncate_page()
 - s/test_bit(BTRFS_ROOT_REF_COWS, &root->state)/root->ref_cows/
 - Keep using BUG_ON() for other error cases, as there is no
   btrfs_abort_transaction()
 - Adjust context]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -88,6 +88,7 @@ static unsigned char btrfs_type_by_mode[
 };
 
 static int btrfs_setsize(struct inode *inode, loff_t newsize);
+static int btrfs_truncate_page(struct address_space *mapping, loff_t from);
 static int btrfs_truncate(struct inode *inode);
 static int btrfs_finish_ordered_io(struct inode *inode, u64 start, u64 end);
 static noinline int cow_file_range(struct inode *inode,
@@ -2992,6 +2993,47 @@ out:
 	return err;
 }
 
+static int truncate_inline_extent(struct btrfs_trans_handle *trans,
+				  struct inode *inode,
+				  struct btrfs_path *path,
+				  struct btrfs_key *found_key,
+				  const u64 item_end,
+				  const u64 new_size)
+{
+	struct extent_buffer *leaf = path->nodes[0];
+	int slot = path->slots[0];
+	struct btrfs_file_extent_item *fi;
+	u32 size = (u32)(new_size - found_key->offset);
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+
+	fi = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item);
+
+	if (btrfs_file_extent_compression(leaf, fi) != BTRFS_COMPRESS_NONE) {
+		loff_t offset = new_size;
+
+		/*
+		 * Zero out the remaining of the last page of our inline extent,
+		 * instead of directly truncating our inline extent here - that
+		 * would be much more complex (decompressing all the data, then
+		 * compressing the truncated data, which might be bigger than
+		 * the size of the inline extent, resize the extent, etc).
+		 * We release the path because to get the page we might need to
+		 * read the extent item from disk (data not in the page cache).
+		 */
+		btrfs_release_path(path);
+		return btrfs_truncate_page(inode->i_mapping, offset);
+	}
+
+	btrfs_set_file_extent_ram_bytes(leaf, fi, size);
+	size = btrfs_file_extent_calc_inline_size(size);
+	btrfs_truncate_item(trans, root, path, size, 1);
+
+	if (root->ref_cows)
+		inode_sub_bytes(inode, item_end + 1 - new_size);
+
+	return 0;
+}
+
 /*
  * this can truncate away extent items, csum items and directory items.
  * It starts at a high offset and removes keys until it can't find
@@ -3153,28 +3195,30 @@ search_again:
 			 * special encodings
 			 */
 			if (!del_item &&
-			    btrfs_file_extent_compression(leaf, fi) == 0 &&
 			    btrfs_file_extent_encryption(leaf, fi) == 0 &&
 			    btrfs_file_extent_other_encoding(leaf, fi) == 0) {
-				u32 size = new_size - found_key.offset;
-
-				if (root->ref_cows) {
-					inode_sub_bytes(inode, item_end + 1 -
-							new_size);
-				}
 
 				/*
-				 * update the ram bytes to properly reflect
-				 * the new size of our item
+				 * Need to release path in order to truncate a
+				 * compressed extent. So delete any accumulated
+				 * extent items so far.
 				 */
-				btrfs_set_file_extent_ram_bytes(leaf, fi, size);
-				size =
-				    btrfs_file_extent_calc_inline_size(size);
-				ret = btrfs_truncate_item(trans, root, path,
-							  size, 1);
+				if (btrfs_file_extent_compression(leaf, fi) !=
+				    BTRFS_COMPRESS_NONE && pending_del_nr) {
+					err = btrfs_del_items(trans, root, path,
+							      pending_del_slot,
+							      pending_del_nr);
+					BUG_ON(err);
+					pending_del_nr = 0;
+				}
+
+				err = truncate_inline_extent(trans, inode,
+							     path, &found_key,
+							     item_end,
+							     new_size);
+				BUG_ON(err);
 			} else if (root->ref_cows) {
-				inode_sub_bytes(inode, item_end + 1 -
-						found_key.offset);
+				inode_sub_bytes(inode, item_end + 1 - new_size);
 			}
 		}
 delete:


  parent reply	other threads:[~2015-11-24 22:37 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-24 22:33 [PATCH 3.2 00/52] 3.2.74-rc1 review Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 02/52] PCI: Use function 0 VPD for identical functions, regular VPD for others Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 11/52] Btrfs: fix file corruption and data loss after cloning inline extents Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 29/52] ipv6: fix tunnel error handling Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 12/52] iommu/vt-d: Fix ATSR handling for Root-Complex integrated endpoints Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 36/52] perf: Fix inherited events vs. tracepoint filters Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 45/52] binfmt_elf: Don't clobber passed executable's file header Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 19/52] staging: rtl8712: Add device ID for Sitecom WLA2100 Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 27/52] megaraid_sas : SMAP restriction--do not access user memory from IOCTL code Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 01/52] PCI: Fix devfn for VPD access through function 0 Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 51/52] net: avoid NULL deref in inet_ctl_sock_destroy() Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 28/52] recordmcount: Fix endianness handling bug for nop_mcount Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 37/52] scsi_sysfs: Fix queue_ramp_up_period return code Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 14/52] Btrfs: don't use ram_bytes for uncompressed inline items Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 40/52] net: fix a race in dst_release() Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 50/52] ipmr: fix possible race resulting from improper usage of IP_INC_STATS_BH() in preemptible context Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 03/52] mac80211: fix driver RSSI event calculations Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 04/52] HID: core: Avoid uninitialized buffer access Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 07/52] mtd: mtdpart: fix add_mtd_partitions error path Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 49/52] RDS-TCP: Recover correctly from pskb_pull()/pksb_trim() failure in rds_tcp_data_recv Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 23/52] megaraid_sas: Do not use PAGE_SIZE for max_sectors Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 48/52] irda: precedence bug in irlmp_seq_hb_idx() Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 32/52] scsi: restart list search after unlock in scsi_remove_target Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 46/52] fs: make dumpable=2 require fully qualified path Ben Hutchings
2015-11-25  2:06   ` James Morris
2015-11-25 17:57     ` Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 33/52] x86/cpu: Call verify_cpu() after having entered long mode too Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 24/52] can: Use correct type in sizeof() in nla_put() Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 31/52] firewire: ohci: fix JMicron JMB38x IT context discovery Ben Hutchings
2015-11-24 23:48   ` Stefan Richter
2015-11-24 22:33 ` [PATCH 3.2 21/52] MIPS: atomic: Fix comment describing atomic64_add_unless's return value Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 16/52] ext4, jbd2: ensure entering into panic after recording an error in superblock Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 17/52] Bluetooth: ath3k: Add new AR3012 0930:021c id Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 10/52] Btrfs: added helper btrfs_next_item() Ben Hutchings
2015-11-24 22:33 ` Ben Hutchings [this message]
2015-11-24 22:33 ` [PATCH 3.2 22/52] ALSA: hda - Disable 64bit address for Creative HDA controllers Ben Hutchings
2015-11-25 23:05   ` Luis Henriques
2015-11-26  0:34     ` Ben Hutchings
2015-11-26 10:30       ` Luis Henriques
2015-11-24 22:33 ` [PATCH 3.2 18/52] Bluetooth: ath3k: Add support of AR3012 0cf3:817b device Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 13/52] ARM: pxa: remove incorrect __init annotation on pxa27x_set_pwrmode Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 39/52] scsi: Fix a bdi reregistration race Ben Hutchings
2015-11-24 22:39   ` Bart Van Assche
2015-11-25 18:00     ` Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 42/52] FS-Cache: Increase reference of parent after registering, netfs success Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 43/52] FS-Cache: Don't override netfs's primary_index if registering failed Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 25/52] mtd: blkdevs: fix potential deadlock + lockdep warnings Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 35/52] Btrfs: fix race leading to BUG_ON when running delalloc for nodatacow Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 26/52] crypto: algif_hash - Only export and import on sockets with data Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 52/52] splice: sendfile() at once fails for big files Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 09/52] packet: fix match_fanout_group() Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 30/52] ALSA: hda - Apply pin fixup for HP ProBook 6550b Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 47/52] fs: if a coredump already exists, unlink and recreate with O_EXCL Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 20/52] ACPI: Use correct IRQ when uninstalling ACPI interrupt handler Ben Hutchings
2015-11-25  2:37   ` Chen, Yu C
2015-11-24 22:33 ` [PATCH 3.2 06/52] mwifiex: fix mwifiex_rdeeprom_read() Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 41/52] KVM: svm: unconditionally intercept #DB Ben Hutchings
2015-11-25 11:31   ` Paolo Bonzini
2015-11-25 17:56     ` Ben Hutchings
2015-11-25 18:06       ` Paolo Bonzini
2015-11-24 22:33 ` [PATCH 3.2 34/52] Btrfs: fix race leading to incorrect item deletion when dropping extents Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 44/52] FS-Cache: Handle a write to the page immediately beyond the EOF marker Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 05/52] wm831x_power: Use IRQF_ONESHOT to request threaded IRQs Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 08/52] devres: fix a for loop bounds check Ben Hutchings
2015-11-24 22:33 ` [PATCH 3.2 38/52] Btrfs: fix race when listing an inode's xattrs Ben Hutchings
2015-11-25 23:11   ` Luis Henriques
2015-11-26  0:39     ` Ben Hutchings
2015-11-26  9:39       ` Filipe Manana
2015-11-25  2:22 ` [PATCH 3.2 00/52] 3.2.74-rc1 review Guenter Roeck
2015-11-25 17:57   ` Ben Hutchings
2015-11-25 17:43 ` Ben Hutchings

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=lsq.1448404439.200660671@decadent.org.uk \
    --to=ben@decadent.org.uk \
    --cc=akpm@linux-foundation.org \
    --cc=fdmanana@suse.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome