From: Darrick J. Wong <djwong@xxxxxxxxxx>
XXX DO NOT APPLY
While running xfs/297 and generic/642, I noticed a crash in
xfs_attri_item_relog when it tries to copy the attr name to the new
xattri log item. I think what happened here was that we called
->iop_commit on the old attri item (which nulls out the pointers) as
part of a log force at the same time that a chained attr operation was
ongoing. The system was busy enough that at some later point, the defer
ops operation decided it was necessary to relog the attri log item, but
as we've detached the name buffer from the old attri log item, we can't
copy it to the new one, and kaboom.
I think speaks to a broader refcounting problem with LARP mode -- the
attr log item needs to be able to retain a reference to the name and
value buffers until the log items have completely cleared the log. I
think it might be possible that the setxattr code can return to
userspace before the CIL actually formats and commits the log item,
leading to a UAF, but I don't really have the time to figure that one
out without external help.
Skipping the memcpy is /not/ the correct solution here -- that means
we'll relog the xattri with zeroed names and values, which breaks log
recovery.
Singed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
---
fs/xfs/xfs_attr_item.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index fb84f71388c4..47c3c44e375d 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -670,12 +670,24 @@ xfs_attri_item_relog(
new_attrp->alfi_name_len = old_attrp->alfi_name_len;
new_attrp->alfi_attr_filter = old_attrp->alfi_attr_filter;
- memcpy(new_attrip->attri_name, old_attrip->attri_name,
- new_attrip->attri_name_len);
+ if (old_attrip->attri_name) {
+ memcpy(new_attrip->attri_name, old_attrip->attri_name,
+ new_attrip->attri_name_len);
+ } else {
+ xfs_emerg(tp->t_mountp, "%s namelen 0x%x name NULL!", __func__, new_attrip->attri_name_len);
+ dump_stack();
+ }
- if (new_attrip->attri_value_len > 0)
- memcpy(new_attrip->attri_value, old_attrip->attri_value,
- new_attrip->attri_value_len);
+ if (new_attrip->attri_value_len > 0) {
+ if (old_attrip->attri_value) {
+ memcpy(new_attrip->attri_value,
+ old_attrip->attri_value,
+ new_attrip->attri_value_len);
+ } else {
+ xfs_emerg(tp->t_mountp, "%s valuelen 0x%x value NULL!", __func__, new_attrip->attri_value_len);
+ dump_stack();
+ }
+ }
xfs_trans_add_item(tp, &new_attrip->attri_item);
set_bit(XFS_LI_DIRTY, &new_attrip->attri_item.li_flags);
