FAILED: patch "[PATCH] xfs: bounds-check buffer log item's dirty bitmap" failed to apply to 6.1-stable tree
Subsystems (2)
[email protected]
maintainerThe patch below does not apply to the 6.1-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
To reproduce the conflict and resubmit, you may use the following commands:
git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.1.y
git checkout FETCH_HEAD
git cherry-pick -x 813f8136a2ce1fee266d02a7df73db6e8a541604
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2026082046-trophy-useable-6a71@gregkh' --subject-prefix 'PATCH 6.1.y' 'HEAD^..'
Possible dependencies:
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 813f8136a2ce1fee266d02a7df73db6e8a541604 Mon Sep 17 00:00:00 2001
From: Ibrahim Hashimov <[email protected]>
Date: Wed, 15 Jul 2026 09:17:23 +0200
Subject: [PATCH] xfs: bounds-check buffer log item's dirty bitmap
xlog_recover_do_reg_buffer() replays each dirty region described by a buffer log item's bitmap into the buffer read for that item:
memcpy(xfs_buf_offset(bp, (uint)bit << XFS_BLF_SHIFT), item->ri_buf[i].iov_base, nbits << XFS_BLF_SHIFT);
The destination offset (bit/nbits, from the logged dirty bitmap) and the buffer size (from the logged blf_len) are both attacker-controlled and otherwise unrelated, yet the only thing bounding the copy is an ASSERT(), which compiles away on production kernels. A crafted image logging a small blf_len together with a bitmap bit past the end of that buffer drives the memcpy() past the buffer's allocation, corrupting adjacent kernel heap during mount-time log recovery. This is reachable by anyone who can get a crafted image mounted -- the malicious-filesystem threat model XFS already guards against elsewhere.
Turn the ASSERT() into a real XFS_IS_CORRUPT() check that aborts recovery of the buffer with -EFSCORRUPTED, consistent with the validate-and-fail idiom already used in xlog_recover_do_inode_buffer() and xfs_dquot_item_recover.c. xlog_recover_do_reg_buffer() therefore becomes STATIC int and its three callers propagate the error.
Found and confirmed with KASAN on a CONFIG_XFS_DEBUG=n build: the crafted image trips a slab-out-of-bounds write before this change and fails recovery cleanly with -EFSCORRUPTED after it.
fs/xfs/xfs_buf_item_recover.c +41 -16
@@ -461,7 +461,7 @@ xlog_recover_validate_buf_type(* given buffer. The bitmap in the buf log format structure indicates * where to place the logged data. */ -STATIC void +STATIC int xlog_recover_do_reg_buffer( struct xfs_mount *mp, struct xlog_recover_item *item,
@@ -489,8 +489,24 @@ xlog_recover_do_reg_buffer(ASSERT (nbits >0 );ASSERT (item->ri_buf[i].iov_base !=NULL );ASSERT (item->ri_buf[i].iov_len %XFS_BLF_CHUNK ==0 ); -ASSERT (BBTOB (bp->b_length) >= - ((uint )bit <<XFS_BLF_SHIFT ) + (nbits <<XFS_BLF_SHIFT )); +/* +* The bitmap is only trustworthy to the extent that it +* describes a region that actually fits inside the buffer we +* read in based on the (attacker-controlled) blf_len. Do not +* rely on an ASSERT() for this -- it compiles away entirely on +* non-DEBUG kernels, which is exactly where this matters, so +* validate it for real and abort recovery of this buffer rather +* than copying past the end of it. +*/ +if (XFS_IS_CORRUPT (mp,BBTOB (bp->b_length) < + ((uint )bit <<XFS_BLF_SHIFT ) + + (nbits <<XFS_BLF_SHIFT ))) { +xfs_alert (mp, +"Bad buffer log item dirty bitmap (bit %d, nbits %d) for %d-byte buffer at daddr 0x%llx." , + bit, nbits,BBTOB (bp->b_length), +xfs_buf_daddr (bp)); +return -EFSCORRUPTED ; + }/* * The dirty regions logged in the buffer, even though
@@ -544,6 +560,7 @@ xlog_recover_do_reg_buffer(ASSERT (i == item->ri_total);xlog_recover_validate_buf_type (mp, bp, buf_f, current_lsn); +return 0 ; }/*
@@ -552,10 +569,10 @@ xlog_recover_do_reg_buffer(* (ie. USR or GRP), then just toss this buffer away; don't recover it. * Else, treat it as a regular buffer and do recovery. * -* Return false if the buffer was tossed and true if we recovered the buffer to -* indicate to the caller if the buffer needs writing. +* Return 0 if the buffer was not recovered (tossed), 1 if it was recovered and +* needs writing, or a negative errno if recovery of the buffer failed. */ -STATIC bool +STATIC int xlog_recover_do_dquot_buffer( struct xfs_mount *mp, struct xlog *log,
@@ -564,6 +581,7 @@ xlog_recover_do_dquot_buffer( struct xfs_buf_log_format *buf_f) {uint type; +int error;trace_xfs_log_recover_buf_dquot_buf (log, buf_f);
@@ -571,7 +589,7 @@ xlog_recover_do_dquot_buffer(* Filesystems are required to send in quota flags at mount time. */ if (!mp->m_qflags) -return false; +return 0 ; type =0 ;if (buf_f->blf_flags &XFS_BLF_UDQUOT_BUF )
@@ -584,10 +602,12 @@ xlog_recover_do_dquot_buffer(* This type of quotas was turned off, so ignore this buffer */ if (log->l_quotaoffs_flag & type) -return false; +return 0 ; -xlog_recover_do_reg_buffer (mp, item, bp, buf_f,NULLCOMMITLSN ); -return true; + error =xlog_recover_do_reg_buffer (mp, item, bp, buf_f,NULLCOMMITLSN ); +if (error) +return error; +return 1 ; }/*
@@ -724,7 +744,9 @@ xlog_recover_do_primary_sb_buffer(xfs_rgnumber_t orig_rgcount = mp->m_sb.sb_rgcount;int error; -xlog_recover_do_reg_buffer (mp, item, bp, buf_f, current_lsn); + error =xlog_recover_do_reg_buffer (mp, item, bp, buf_f, current_lsn); +if (error) +return error;if (orig_agcount ==0 ) {xfs_alert (mp,"Trying to grow file system without AGs" );
@@ -1081,11 +1103,11 @@ xlog_recover_buf_commit_pass2( goto out_release; }else if (buf_f->blf_flags & (XFS_BLF_UDQUOT_BUF |XFS_BLF_PDQUOT_BUF |XFS_BLF_GDQUOT_BUF )) { -bool dirty; - - dirty =xlog_recover_do_dquot_buffer (mp, log, item, bp, buf_f); -if (!dirty) + error =xlog_recover_do_dquot_buffer (mp, log, item, bp, buf_f); +if (error <=0 ) goto out_release; +/* write dirty buffer */ + error =0 ; }else if ((xfs_blft_from_flags (buf_f) &XFS_BLFT_SB_BUF ) &&xfs_buf_daddr (bp) ==0 ) { error = xlog_recover_do_primary_sb_buffer(mp, item, bp, buf_f,
@@ -1105,7 +1127,10 @@ xlog_recover_buf_commit_pass2(xfs_buf_relse (rtsb_bp); } }else { -xlog_recover_do_reg_buffer (mp, item, bp, buf_f, current_lsn); + error =xlog_recover_do_reg_buffer (mp, item, bp, buf_f, + current_lsn); +if (error) + goto out_release; }/*
Sasha Levin
maintainerPATCH 1/2 xfs: don't use a xfs_log_iovec for ri_buf in log recovery
From: Christoph Hellwig <[email protected]>
[ Upstream commit ded74fddcaf685a9440c5612f7831d0c4c1473ca ]
ri_buf just holds a pointer/len pair and is not a log iovec used for writing to the log. Switch to use a kvec instead.
Stable-dep-of: 813f8136a2ce ("xfs: bounds-check buffer log item's dirty bitmap")
--- fs/xfs/libxfs/xfs_log_recover.h | 4 +-- fs/xfs/xfs_attr_item.c | 24 +++++++++--------- fs/xfs/xfs_bmap_item.c | 18 +++++++------- fs/xfs/xfs_buf_item.c | 8 +++--- fs/xfs/xfs_buf_item.h | 2 +- fs/xfs/xfs_buf_item_recover.c | 38 ++++++++++++++--------------- fs/xfs/xfs_dquot_item_recover.c | 20 +++++++-------- fs/xfs/xfs_extfree_item.c | 43 ++++++++++++++++++--------------- fs/xfs/xfs_icreate_item.c | 2 +- fs/xfs/xfs_inode_item.c | 6 ++--- fs/xfs/xfs_inode_item.h | 4 +-- fs/xfs/xfs_inode_item_recover.c | 26 ++++++++++---------- fs/xfs/xfs_log_recover.c | 16 ++++++------ fs/xfs/xfs_refcount_item.c | 16 ++++++------ fs/xfs/xfs_rmap_item.c | 16 ++++++------ fs/xfs/xfs_trans.h | 1 - 16 files changed, 123 insertions(+), 121 deletions(-)
fs/xfs/libxfs/xfs_log_recover.h +2 -2
@@ -95,7 +95,7 @@ struct xlog_recover_item {struct list_head ri_list;int ri_cnt;/* count of regions found */ int ri_total;/* total regions */ -struct xfs_log_iovec *ri_buf;/* ptr to regions buffer */ +struct kvec *ri_buf;/* ptr to regions buffer */ const struct xlog_recover_item_ops *ri_ops; };
@@ -108,7 +108,7 @@ struct xlog_recover {struct list_head r_itemq;/* q for items */ }; -#define ITEM_TYPE (i) (*(unsigned short *)(i)->ri_buf[0].i_addr) +#define ITEM_TYPE (i) (*(unsigned short *)(i)->ri_buf[0].iov_base)#define XLOG_RECOVER_CRCPASS 0#define XLOG_RECOVER_PASS1 1
fs/xfs/xfs_attr_item.c +12 -12
@@ -721,13 +721,13 @@ xlog_recover_attri_commit_pass2(/* Validate xfs_attri_log_format before the large memory allocation */ len =sizeof (struct xfs_attri_log_format ); -if (item->ri_buf[i].i_len != len) { +if (item->ri_buf[i].iov_len != len) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; } - attri_formatp = item->ri_buf[i].i_addr; + attri_formatp = item->ri_buf[i].iov_base;if (!xfs_attri_validate (mp, attri_formatp)) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, attri_formatp, len);
@@ -762,14 +762,14 @@ xlog_recover_attri_commit_pass2( i++;/* Validate the attr name */ -if (item->ri_buf[i].i_len != +if (item->ri_buf[i].iov_len !=xlog_calc_iovec_len (attri_formatp->alfi_name_len)) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, attri_formatp, len);return -EFSCORRUPTED ; } - attr_name = item->ri_buf[i].i_addr; + attr_name = item->ri_buf[i].iov_base;if (!xfs_attr_namecheck (attr_name, attri_formatp->alfi_name_len)) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, attri_formatp, len);
@@ -779,14 +779,14 @@ xlog_recover_attri_commit_pass2(/* Validate the attr value, if present */ if (attri_formatp->alfi_value_len !=0 ) { -if (item->ri_buf[i].i_len !=xlog_calc_iovec_len (attri_formatp->alfi_value_len)) { +if (item->ri_buf[i].iov_len !=xlog_calc_iovec_len (attri_formatp->alfi_value_len)) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, - item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, + item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; } - attr_value = item->ri_buf[i].i_addr; + attr_value = item->ri_buf[i].iov_base; i++; }
@@ -908,10 +908,10 @@ xlog_recover_attrd_commit_pass2( {struct xfs_attrd_log_format *attrd_formatp; - attrd_formatp = item->ri_buf[0 ].i_addr; -if (item->ri_buf[0 ].i_len !=sizeof (struct xfs_attrd_log_format )) { + attrd_formatp = item->ri_buf[0 ].iov_base; +if (item->ri_buf[0 ].iov_len !=sizeof (struct xfs_attrd_log_format )) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , log->l_mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }
fs/xfs/xfs_bmap_item.c +9 -9
@@ -623,24 +623,24 @@ xlog_recover_bui_commit_pass2(struct xfs_bui_log_format *bui_formatp;size_t len; - bui_formatp = item->ri_buf[0 ].i_addr; + bui_formatp = item->ri_buf[0 ].iov_base; -if (item->ri_buf[0 ].i_len <xfs_bui_log_format_sizeof (0 )) { +if (item->ri_buf[0 ].iov_len <xfs_bui_log_format_sizeof (0 )) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }if (bui_formatp->bui_nextents !=XFS_BUI_MAX_FAST_EXTENTS ) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; } len =xfs_bui_log_format_sizeof (bui_formatp->bui_nextents); -if (item->ri_buf[0 ].i_len != len) { +if (item->ri_buf[0 ].iov_len != len) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }
@@ -677,10 +677,10 @@ xlog_recover_bud_commit_pass2( {struct xfs_bud_log_format *bud_formatp; - bud_formatp = item->ri_buf[0 ].i_addr; -if (item->ri_buf[0 ].i_len !=sizeof (struct xfs_bud_log_format )) { + bud_formatp = item->ri_buf[0 ].iov_base; +if (item->ri_buf[0 ].iov_len !=sizeof (struct xfs_bud_log_format )) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , log->l_mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }
fs/xfs/xfs_buf_item.c +4 -4
@@ -35,16 +35,16 @@ static inline struct xfs_buf_log_item *BUF_ITEM(struct xfs_log_item *lip)/* Is this log iovec plausibly large enough to contain the buffer log format? */ bool xfs_buf_log_check_iovec ( -struct xfs_log_iovec *iovec) +struct kvec *iovec) { -struct xfs_buf_log_format *blfp = iovec->i_addr; +struct xfs_buf_log_format *blfp = iovec->iov_base;char *bmp_end;char *item_end; -if (offsetof(struct xfs_buf_log_format , blf_data_map) > iovec->i_len) +if (offsetof(struct xfs_buf_log_format , blf_data_map) > iovec->iov_len)return false; - item_end = (char *)iovec->i_addr + iovec->i_len; + item_end = (char *)iovec->iov_base + iovec->iov_len; bmp_end = (char *)&blfp->blf_data_map[blfp->blf_map_size];return bmp_end <= item_end; }
fs/xfs/xfs_buf_item.h +1 -1
@@ -67,7 +67,7 @@ static inline void xfs_buf_dquot_io_fail(struct xfs_buf *bp) }#endif /* CONFIG_XFS_QUOTA */ void xfs_buf_iodone (struct xfs_buf *); -bool xfs_buf_log_check_iovec (struct xfs_log_iovec *iovec); +bool xfs_buf_log_check_iovec (struct kvec *iovec);extern struct kmem_cache *xfs_buf_item_cache;
fs/xfs/xfs_buf_item_recover.c +19 -19
@@ -154,7 +154,7 @@ STATIC enum xlog_recover_reorderxlog_recover_buf_reorder ( struct xlog_recover_item *item) { -struct xfs_buf_log_format *buf_f = item->ri_buf[0 ].i_addr; +struct xfs_buf_log_format *buf_f = item->ri_buf[0 ].iov_base;if (buf_f->blf_flags &XFS_BLF_CANCEL )return XLOG_REORDER_CANCEL_LIST ;
@@ -168,7 +168,7 @@ xlog_recover_buf_ra_pass2(struct xlog *log, struct xlog_recover_item *item) { - struct xfs_buf_log_format *buf_f = item->ri_buf[0].i_addr; + struct xfs_buf_log_format *buf_f = item->ri_buf[0].iov_base;xlog_buf_readahead (log, buf_f->blf_blkno, buf_f->blf_len,NULL ); }
@@ -182,11 +182,11 @@ xlog_recover_buf_commit_pass1(struct xlog *log, struct xlog_recover_item *item) { - struct xfs_buf_log_format *bf = item->ri_buf[0].i_addr; + struct xfs_buf_log_format *bf = item->ri_buf[0].iov_base;if (!xfs_buf_log_check_iovec (&item->ri_buf[0 ])) { -xfs_err (log->l_mp,"bad buffer log item size (%d)" , - item->ri_buf[0 ].i_len); +xfs_err (log->l_mp,"bad buffer log item size (%zd)" , + item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }
@@ -467,8 +467,8 @@ xlog_recover_do_reg_buffer( nbits =xfs_contig_bits (buf_f->blf_data_map, buf_f->blf_map_size, bit);ASSERT (nbits >0 ); -ASSERT (item->ri_buf[i].i_addr !=NULL ); -ASSERT (item->ri_buf[i].i_len %XFS_BLF_CHUNK ==0 ); +ASSERT (item->ri_buf[i].iov_base !=NULL ); +ASSERT (item->ri_buf[i].iov_len %XFS_BLF_CHUNK ==0 );ASSERT (BBTOB (bp->b_length) >= ((uint )bit <<XFS_BLF_SHIFT ) + (nbits <<XFS_BLF_SHIFT ));
@@ -480,8 +480,8 @@ xlog_recover_do_reg_buffer(* the log. Hence we need to trim nbits back to the length of * the current region being copied out of the log. */ -if (item->ri_buf[i].i_len < (nbits <<XFS_BLF_SHIFT )) - nbits = item->ri_buf[i].i_len >>XFS_BLF_SHIFT ; +if (item->ri_buf[i].iov_len < (nbits <<XFS_BLF_SHIFT )) + nbits = item->ri_buf[i].iov_len >>XFS_BLF_SHIFT ;/* * Do a sanity check if this is a dquot buffer. Just checking
@@ -491,18 +491,18 @@ xlog_recover_do_reg_buffer( fa =NULL ;if (buf_f->blf_flags & (XFS_BLF_UDQUOT_BUF |XFS_BLF_PDQUOT_BUF |XFS_BLF_GDQUOT_BUF )) { -if (item->ri_buf[i].i_addr ==NULL ) { +if (item->ri_buf[i].iov_base ==NULL ) {xfs_alert (mp,"XFS: NULL dquot in %s." , __func__); goto next; } -if (item->ri_buf[i].i_len < size_disk_dquot) { +if (item->ri_buf[i].iov_len < size_disk_dquot) {xfs_alert (mp, -"XFS: dquot too small (%d) in %s." , - item->ri_buf[i].i_len, __func__); +"XFS: dquot too small (%zd) in %s." , + item->ri_buf[i].iov_len, __func__); goto next; } - fa =xfs_dquot_verify (mp, item->ri_buf[i].i_addr,-1 ); + fa =xfs_dquot_verify (mp, item->ri_buf[i].iov_base,-1 );if (fa) { xfs_alert(mp, "dquot corrupt at %pS trying to replay into block 0x%llx",
@@ -513,7 +513,7 @@ xlog_recover_do_reg_buffer(memcpy (xfs_buf_offset (bp, (uint )bit <<XFS_BLF_SHIFT ),/* dest */ - item->ri_buf[i].i_addr,/* source */ + item->ri_buf[i].iov_base,/* source */ nbits<<XFS_BLF_SHIFT );/* length */ next: i++;
@@ -649,8 +649,8 @@ xlog_recover_do_inode_buffer(if (next_unlinked_offset < reg_buf_offset)continue ; -ASSERT (item->ri_buf[item_index].i_addr !=NULL ); -ASSERT ((item->ri_buf[item_index].i_len %XFS_BLF_CHUNK ) ==0 ); +ASSERT (item->ri_buf[item_index].iov_base !=NULL ); +ASSERT ((item->ri_buf[item_index].iov_len %XFS_BLF_CHUNK ) ==0 );ASSERT ((reg_buf_offset + reg_buf_bytes) <=BBTOB (bp->b_length));/*
@@ -658,7 +658,7 @@ xlog_recover_do_inode_buffer(* current di_next_unlinked field. Extract its value * and copy it to the buffer copy. */ - logged_nextp = item->ri_buf[item_index].i_addr + + logged_nextp = item->ri_buf[item_index].iov_base + next_unlinked_offset - reg_buf_offset;if (XFS_IS_CORRUPT (mp, *logged_nextp ==0 )) { xfs_alert(mp,
@@ -887,7 +887,7 @@ xlog_recover_buf_commit_pass2( struct xlog_recover_item *item, xfs_lsn_t current_lsn) { -struct xfs_buf_log_format *buf_f = item->ri_buf[0 ].i_addr; +struct xfs_buf_log_format *buf_f = item->ri_buf[0 ].iov_base;struct xfs_mount *mp = log->l_mp;struct xfs_buf *bp;int error;
fs/xfs/xfs_dquot_item_recover.c +10 -10
@@ -34,10 +34,10 @@ xlog_recover_dquot_ra_pass2(if (mp->m_qflags ==0 )return ; - recddq = item->ri_buf[1 ].i_addr; + recddq = item->ri_buf[1 ].iov_base;if (recddq ==NULL )return ; -if (item->ri_buf[1 ].i_len <sizeof (struct xfs_disk_dquot )) +if (item->ri_buf[1 ].iov_len <sizeof (struct xfs_disk_dquot ))return ; type = recddq->d_type &XFS_DQTYPE_REC_MASK ;
@@ -45,7 +45,7 @@ xlog_recover_dquot_ra_pass2(if (log->l_quotaoffs_flag & type)return ; - dq_f = item->ri_buf[0 ].i_addr; + dq_f = item->ri_buf[0 ].iov_base;ASSERT (dq_f);ASSERT (dq_f->qlf_len ==1 );
@@ -79,14 +79,14 @@ xlog_recover_dquot_commit_pass2(if (mp->m_qflags ==0 )return 0 ; - recddq = item->ri_buf[1 ].i_addr; + recddq = item->ri_buf[1 ].iov_base;if (recddq ==NULL ) {xfs_alert (log->l_mp,"NULL dquot in %s." , __func__);return -EFSCORRUPTED ; } -if (item->ri_buf[1 ].i_len <sizeof (struct xfs_disk_dquot )) { -xfs_alert (log->l_mp,"dquot too small (%d) in %s." , - item->ri_buf[1 ].i_len, __func__); +if (item->ri_buf[1 ].iov_len <sizeof (struct xfs_disk_dquot )) { +xfs_alert (log->l_mp,"dquot too small (%zd) in %s." , + item->ri_buf[1 ].iov_len, __func__);return -EFSCORRUPTED ; }
@@ -108,7 +108,7 @@ xlog_recover_dquot_commit_pass2(* The other possibility, of course, is that the quota subsystem was * removed since the last mount - ENOSYS. */ - dq_f = item->ri_buf[0 ].i_addr; + dq_f = item->ri_buf[0 ].iov_base;ASSERT (dq_f); fa =xfs_dquot_verify (mp, recddq, dq_f->qlf_id);if (fa) {
@@ -147,7 +147,7 @@ xlog_recover_dquot_commit_pass2( } } -memcpy (ddq, recddq, item->ri_buf[1 ].i_len); +memcpy (ddq, recddq, item->ri_buf[1 ].iov_len);if (xfs_has_crc (mp)) {xfs_update_cksum ((char *)dqb,sizeof (struct xfs_dqblk ),XFS_DQUOT_CRC_OFF );
@@ -192,7 +192,7 @@ xlog_recover_quotaoff_commit_pass1(struct xlog *log, struct xlog_recover_item *item) { - struct xfs_qoff_logformat *qoff_f = item->ri_buf[0].i_addr; + struct xfs_qoff_logformat *qoff_f = item->ri_buf[0].iov_base;ASSERT (qoff_f);/*
fs/xfs/xfs_extfree_item.c +23 -20
91 lines not on this page · raw message
fs/xfs/xfs_icreate_item.c +1 -1
@@ -158,7 +158,7 @@ xlog_recover_icreate_commit_pass2(int nbufs;int i; - icl = (struct xfs_icreate_log *)item->ri_buf[0 ].i_addr; + icl = (struct xfs_icreate_log *)item->ri_buf[0 ].iov_base;if (icl->icl_type !=XFS_LI_ICREATE ) {xfs_warn (log->l_mp,"xlog_recover_do_icreate_trans: bad type" );return -EINVAL ;
fs/xfs/xfs_inode_item.c +3 -3
@@ -1177,12 +1177,12 @@ xfs_iflush_shutdown_abort(*/ int xfs_inode_item_format_convert ( -struct xfs_log_iovec *buf, +struct kvec *buf,struct xfs_inode_log_format *in_f) { -struct xfs_inode_log_format_32 *in_f32 = buf->i_addr; +struct xfs_inode_log_format_32 *in_f32 = buf->iov_base; -if (buf->i_len !=sizeof (*in_f32)) { +if (buf->iov_len !=sizeof (*in_f32)) {XFS_ERROR_REPORT (__func__,XFS_ERRLEVEL_LOW ,NULL );return -EFSCORRUPTED ; }
fs/xfs/xfs_inode_item.h +2 -2
@@ -46,8 +46,8 @@ extern void xfs_inode_item_init(struct xfs_inode *, struct xfs_mount *);extern void xfs_inode_item_destroy (struct xfs_inode *);extern void xfs_iflush_abort (struct xfs_inode *);extern void xfs_iflush_shutdown_abort (struct xfs_inode *); -extern int xfs_inode_item_format_convert (xfs_log_iovec_t *, -struct xfs_inode_log_format *); +int xfs_inode_item_format_convert (struct kvec *buf, +struct xfs_inode_log_format *in_f);extern struct kmem_cache *xfs_ili_cache;
fs/xfs/xfs_inode_item_recover.c +13 -13
@@ -28,13 +28,13 @@ xlog_recover_inode_ra_pass2( struct xlog *log, struct xlog_recover_item *item) { -if (item->ri_buf[0 ].i_len ==sizeof (struct xfs_inode_log_format )) { -struct xfs_inode_log_format *ilfp = item->ri_buf[0 ].i_addr; +if (item->ri_buf[0 ].iov_len ==sizeof (struct xfs_inode_log_format )) { +struct xfs_inode_log_format *ilfp = item->ri_buf[0 ].iov_base;xlog_buf_readahead (log, ilfp->ilf_blkno, ilfp->ilf_len, &xfs_inode_buf_ra_ops); }else { -struct xfs_inode_log_format_32 *ilfp = item->ri_buf[0 ].i_addr; +struct xfs_inode_log_format_32 *ilfp = item->ri_buf[0 ].iov_base;xlog_buf_readahead (log, ilfp->ilf_blkno, ilfp->ilf_len, &xfs_inode_buf_ra_ops);
@@ -288,8 +288,8 @@ xlog_recover_inode_commit_pass2(int need_free =0 ;xfs_failaddr_t fa; -if (item->ri_buf[0 ].i_len ==sizeof (struct xfs_inode_log_format )) { - in_f = item->ri_buf[0 ].i_addr; +if (item->ri_buf[0 ].iov_len ==sizeof (struct xfs_inode_log_format )) { + in_f = item->ri_buf[0 ].iov_base; }else { in_f =kmem_alloc (sizeof (struct xfs_inode_log_format ),0 ); need_free =1 ;
@@ -327,7 +327,7 @@ xlog_recover_inode_commit_pass2( error = -EFSCORRUPTED ; goto out_release; } - ldip = item->ri_buf[1 ].i_addr; + ldip = item->ri_buf[1 ].iov_base;if (XFS_IS_CORRUPT (mp, ldip->di_magic !=XFS_DINODE_MAGIC )) { xfs_alert(mp, "%s: Bad inode log record, rec ptr "PTR_FMT", ino %lld",
@@ -432,12 +432,12 @@ xlog_recover_inode_commit_pass2( goto out_release; } isize =xfs_log_dinode_size (mp); -if (unlikely (item->ri_buf[1 ].i_len > isize)) { +if (unlikely (item->ri_buf[1 ].iov_len > isize)) {XFS_CORRUPTION_ERROR ("Bad log dinode size" ,XFS_ERRLEVEL_LOW , mp, ldip,sizeof (*ldip));xfs_alert (mp, -"Bad inode 0x%llx log dinode size 0x%x" , - in_f->ilf_ino, item->ri_buf[1 ].i_len); +"Bad inode 0x%llx log dinode size 0x%zx" , + in_f->ilf_ino, item->ri_buf[1 ].iov_len); error = -EFSCORRUPTED ; goto out_release; }
@@ -460,8 +460,8 @@ xlog_recover_inode_commit_pass2(if (in_f->ilf_size ==2 ) goto out_owner_change; - len = item->ri_buf[2 ].i_len; - src = item->ri_buf[2 ].i_addr; + len = item->ri_buf[2 ].iov_len; + src = item->ri_buf[2 ].iov_base;ASSERT (in_f->ilf_size <=4 );ASSERT ((in_f->ilf_size ==3 ) || (fields &XFS_ILOG_AFORK )); ASSERT(!(fields & XFS_ILOG_DFORK) ||
@@ -498,8 +498,8 @@ xlog_recover_inode_commit_pass2( }else { attr_index =2 ; } - len = item->ri_buf[attr_index].i_len; - src = item->ri_buf[attr_index].i_addr; + len = item->ri_buf[attr_index].iov_len; + src = item->ri_buf[attr_index].iov_base;ASSERT (len ==xlog_calc_iovec_len (in_f->ilf_asize));switch (in_f->ilf_fields &XFS_ILOG_AFORK ) {
fs/xfs/xfs_log_recover.c +8 -8
@@ -2088,15 +2088,15 @@ xlog_recover_add_to_cont_trans( item = list_entry(trans->r_itemq.prev, struct xlog_recover_item, ri_list); - old_ptr = item->ri_buf[item->ri_cnt-1 ].i_addr; - old_len = item->ri_buf[item->ri_cnt-1 ].i_len; + old_ptr = item->ri_buf[item->ri_cnt-1 ].iov_base; + old_len = item->ri_buf[item->ri_cnt-1 ].iov_len; ptr =kvrealloc (old_ptr, old_len, len + old_len,GFP_KERNEL );if (!ptr)return -ENOMEM ;memcpy (&ptr[old_len], dp, len); - item->ri_buf[item->ri_cnt-1 ].i_len += len; - item->ri_buf[item->ri_cnt-1 ].i_addr = ptr; + item->ri_buf[item->ri_cnt-1 ].iov_len += len; + item->ri_buf[item->ri_cnt-1 ].iov_base = ptr;trace_xfs_log_recover_item_add_cont (log, trans, item,0 );return 0 ; }
@@ -2181,7 +2181,7 @@ xlog_recover_add_to_trans( item->ri_total = in_f->ilf_size; item->ri_buf = -kmem_zalloc (item->ri_total *sizeof (xfs_log_iovec_t), +kmem_zalloc (item->ri_total *sizeof (*item->ri_buf),0 ); }
@@ -2195,8 +2195,8 @@ xlog_recover_add_to_trans( }/* Description region is ri_buf[0] */ - item->ri_buf[item->ri_cnt].i_addr = ptr; - item->ri_buf[item->ri_cnt].i_len = len; + item->ri_buf[item->ri_cnt].iov_base = ptr; + item->ri_buf[item->ri_cnt].iov_len = len; item->ri_cnt++;trace_xfs_log_recover_item_add (log, trans, item,0 );return 0 ;
@@ -2220,7 +2220,7 @@ xlog_recover_free_trans(/* Free the regions in the item. */ list_del (&item->ri_list);for (i =0 ; i < item->ri_cnt; i++) -kmem_free (item->ri_buf[i].i_addr); +kmem_free (item->ri_buf[i].iov_base);/* Free the item itself */ kmem_free (item->ri_buf);kmem_free (item);
fs/xfs/xfs_refcount_item.c +8 -8
@@ -650,18 +650,18 @@ xlog_recover_cui_commit_pass2(struct xfs_cui_log_format *cui_formatp;size_t len; - cui_formatp = item->ri_buf[0 ].i_addr; + cui_formatp = item->ri_buf[0 ].iov_base; -if (item->ri_buf[0 ].i_len <xfs_cui_log_format_sizeof (0 )) { +if (item->ri_buf[0 ].iov_len <xfs_cui_log_format_sizeof (0 )) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; } len =xfs_cui_log_format_sizeof (cui_formatp->cui_nextents); -if (item->ri_buf[0 ].i_len != len) { +if (item->ri_buf[0 ].iov_len != len) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }
@@ -698,10 +698,10 @@ xlog_recover_cud_commit_pass2( {struct xfs_cud_log_format *cud_formatp; - cud_formatp = item->ri_buf[0 ].i_addr; -if (item->ri_buf[0 ].i_len !=sizeof (struct xfs_cud_log_format )) { + cud_formatp = item->ri_buf[0 ].iov_base; +if (item->ri_buf[0 ].iov_len !=sizeof (struct xfs_cud_log_format )) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , log->l_mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }
fs/xfs/xfs_rmap_item.c +8 -8
@@ -664,18 +664,18 @@ xlog_recover_rui_commit_pass2(struct xfs_rui_log_format *rui_formatp;size_t len; - rui_formatp = item->ri_buf[0 ].i_addr; + rui_formatp = item->ri_buf[0 ].iov_base; -if (item->ri_buf[0 ].i_len <xfs_rui_log_format_sizeof (0 )) { +if (item->ri_buf[0 ].iov_len <xfs_rui_log_format_sizeof (0 )) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; } len =xfs_rui_log_format_sizeof (rui_formatp->rui_nextents); -if (item->ri_buf[0 ].i_len != len) { +if (item->ri_buf[0 ].iov_len != len) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , mp, - item->ri_buf[0 ].i_addr, item->ri_buf[0 ].i_len); + item->ri_buf[0 ].iov_base, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }
@@ -712,10 +712,10 @@ xlog_recover_rud_commit_pass2( {struct xfs_rud_log_format *rud_formatp; - rud_formatp = item->ri_buf[0 ].i_addr; -if (item->ri_buf[0 ].i_len !=sizeof (struct xfs_rud_log_format )) { + rud_formatp = item->ri_buf[0 ].iov_base; +if (item->ri_buf[0 ].iov_len !=sizeof (struct xfs_rud_log_format )) {XFS_CORRUPTION_ERROR (__func__,XFS_ERRLEVEL_LOW , log->l_mp, - rud_formatp, item->ri_buf[0 ].i_len); + rud_formatp, item->ri_buf[0 ].iov_len);return -EFSCORRUPTED ; }
fs/xfs/xfs_trans.h +0 -1
@@ -15,7 +15,6 @@ struct xfs_efd_log_item;struct xfs_efi_log_item ;struct xfs_inode ;struct xfs_item_ops ; -struct xfs_log_iovec ;struct xfs_mount ;struct xfs_trans ;struct xfs_trans_res ;
Signature · 1 line
2.53.0
Sasha Levin
maintainerPATCH 2/2 xfs: bounds-check buffer log item's dirty bitmap
From: Ibrahim Hashimov <[email protected]>
[ Upstream commit 813f8136a2ce1fee266d02a7df73db6e8a541604 ]
xlog_recover_do_reg_buffer() replays each dirty region described by a buffer log item's bitmap into the buffer read for that item:
memcpy(xfs_buf_offset(bp, (uint)bit << XFS_BLF_SHIFT), item->ri_buf[i].iov_base, nbits << XFS_BLF_SHIFT);
The destination offset (bit/nbits, from the logged dirty bitmap) and the buffer size (from the logged blf_len) are both attacker-controlled and otherwise unrelated, yet the only thing bounding the copy is an ASSERT(), which compiles away on production kernels. A crafted image logging a small blf_len together with a bitmap bit past the end of that buffer drives the memcpy() past the buffer's allocation, corrupting adjacent kernel heap during mount-time log recovery. This is reachable by anyone who can get a crafted image mounted -- the malicious-filesystem threat model XFS already guards against elsewhere.
Turn the ASSERT() into a real XFS_IS_CORRUPT() check that aborts recovery of the buffer with -EFSCORRUPTED, consistent with the validate-and-fail idiom already used in xlog_recover_do_inode_buffer() and xfs_dquot_item_recover.c. xlog_recover_do_reg_buffer() therefore becomes STATIC int and its three callers propagate the error.
Found and confirmed with KASAN on a CONFIG_XFS_DEBUG=n build: the crafted image trips a slab-out-of-bounds write before this change and fails recovery cleanly with -EFSCORRUPTED after it.
--- fs/xfs/xfs_buf_item_recover.c | 53 +++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 15 deletions(-)
fs/xfs/xfs_buf_item_recover.c +38 -15
@@ -441,7 +441,7 @@ xlog_recover_validate_buf_type(* given buffer. The bitmap in the buf log format structure indicates * where to place the logged data. */ -STATIC void +STATIC int xlog_recover_do_reg_buffer( struct xfs_mount *mp, struct xlog_recover_item *item,
@@ -469,8 +469,24 @@ xlog_recover_do_reg_buffer(ASSERT (nbits >0 );ASSERT (item->ri_buf[i].iov_base !=NULL );ASSERT (item->ri_buf[i].iov_len %XFS_BLF_CHUNK ==0 ); -ASSERT (BBTOB (bp->b_length) >= - ((uint )bit <<XFS_BLF_SHIFT ) + (nbits <<XFS_BLF_SHIFT )); +/* +* The bitmap is only trustworthy to the extent that it +* describes a region that actually fits inside the buffer we +* read in based on the (attacker-controlled) blf_len. Do not +* rely on an ASSERT() for this -- it compiles away entirely on +* non-DEBUG kernels, which is exactly where this matters, so +* validate it for real and abort recovery of this buffer rather +* than copying past the end of it. +*/ +if (XFS_IS_CORRUPT (mp,BBTOB (bp->b_length) < + ((uint )bit <<XFS_BLF_SHIFT ) + + (nbits <<XFS_BLF_SHIFT ))) { +xfs_alert (mp, +"Bad buffer log item dirty bitmap (bit %d, nbits %d) for %d-byte buffer at daddr 0x%llx." , + bit, nbits,BBTOB (bp->b_length), +xfs_buf_daddr (bp)); +return -EFSCORRUPTED ; + }/* * The dirty regions logged in the buffer, even though
@@ -524,6 +540,7 @@ xlog_recover_do_reg_buffer(ASSERT (i == item->ri_total);xlog_recover_validate_buf_type (mp, bp, buf_f, current_lsn); +return 0 ; }/*
@@ -532,10 +549,10 @@ xlog_recover_do_reg_buffer(* (ie. USR or GRP), then just toss this buffer away; don't recover it. * Else, treat it as a regular buffer and do recovery. * -* Return false if the buffer was tossed and true if we recovered the buffer to -* indicate to the caller if the buffer needs writing. +* Return 0 if the buffer was not recovered (tossed), 1 if it was recovered and +* needs writing, or a negative errno if recovery of the buffer failed. */ -STATIC bool +STATIC int xlog_recover_do_dquot_buffer( struct xfs_mount *mp, struct xlog *log,
@@ -544,6 +561,7 @@ xlog_recover_do_dquot_buffer( struct xfs_buf_log_format *buf_f) {uint type; +int error;trace_xfs_log_recover_buf_dquot_buf (log, buf_f);
@@ -551,7 +569,7 @@ xlog_recover_do_dquot_buffer(* Filesystems are required to send in quota flags at mount time. */ if (!mp->m_qflags) -return false; +return 0 ; type =0 ;if (buf_f->blf_flags &XFS_BLF_UDQUOT_BUF )
@@ -564,10 +582,12 @@ xlog_recover_do_dquot_buffer(* This type of quotas was turned off, so ignore this buffer */ if (log->l_quotaoffs_flag & type) -return false; +return 0 ; -xlog_recover_do_reg_buffer (mp, item, bp, buf_f,NULLCOMMITLSN ); -return true; + error =xlog_recover_do_reg_buffer (mp, item, bp, buf_f,NULLCOMMITLSN ); +if (error) +return error; +return 1 ; }/*
@@ -962,13 +982,16 @@ xlog_recover_buf_commit_pass2( goto out_release; }else if (buf_f->blf_flags & (XFS_BLF_UDQUOT_BUF |XFS_BLF_PDQUOT_BUF |XFS_BLF_GDQUOT_BUF )) { -bool dirty; - - dirty =xlog_recover_do_dquot_buffer (mp, log, item, bp, buf_f); -if (!dirty) + error =xlog_recover_do_dquot_buffer (mp, log, item, bp, buf_f); +if (error <=0 ) goto out_release; +/* write dirty buffer */ + error =0 ; }else { -xlog_recover_do_reg_buffer (mp, item, bp, buf_f, current_lsn); + error =xlog_recover_do_reg_buffer (mp, item, bp, buf_f, + current_lsn); +if (error) + goto out_release; }/*
Signature · 1 line
2.53.0