[PATCH 1/5] apfsck: check length of xattr record values

[Ernesto A. Fernández]

While parsing the catalog tree, check that the length of each xattr
record is consistent with its fields.

Signed-off-by: Ernesto A. Fernández <ernesto.mn...@gmail.com>
---
apfsck/Makefile | 2 +-
apfsck/btree.c | 4 ++++
apfsck/xattr.c | 42 ++++++++++++++++++++++++++++++++++++++++++
apfsck/xattr.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 97 insertions(+), 1 deletion(-)
create mode 100644 apfsck/xattr.c
create mode 100644 apfsck/xattr.h

diff --git a/apfsck/Makefile b/apfsck/Makefile
index cef4a1e..3beeb38 100644
--- a/apfsck/Makefile
+++ b/apfsck/Makefile
@@ -1,5 +1,5 @@
SRCS = apfsck.c btree.c crc32c.c dir.c extents.c \
- inode.c key.c object.c super.c unicode.c
+ inode.c key.c object.c super.c unicode.c xattr.c
OBJS = $(SRCS:.c=.o)
DEPS = $(SRCS:.c=.d)

diff --git a/apfsck/btree.c b/apfsck/btree.c
index f2200cf..419e96a 100644
--- a/apfsck/btree.c
+++ b/apfsck/btree.c
@@ -18,6 +18,7 @@
#include "object.h"
#include "super.h"
#include "types.h"
+#include "xattr.h"

/**
* node_is_valid - Check basic sanity of the node index
@@ -512,6 +513,9 @@ static void parse_cat_record(void *key, void *val, int len)
case APFS_TYPE_SIBLING_LINK:
parse_sibling_record(key, val, len);
break;
+ case APFS_TYPE_XATTR:
+ parse_xattr_record(key, val, len);
+ break;
default:
break;
}
diff --git a/apfsck/xattr.c b/apfsck/xattr.c
new file mode 100644
index 0000000..1ec3627
--- /dev/null
+++ b/apfsck/xattr.c
@@ -0,0 +1,42 @@
+/*
+ * apfsprogs/apfsck/xattr.c
+ *
+ * Copyright (C) 2018 Ernesto A. Fernández <ernesto.mn...@gmail.com>
+ */
+
+#include "apfsck.h"
+#include "key.h"
+#include "types.h"
+#include "xattr.h"
+
+/**
+ * parse_xattr_record - Parse a xattr record value and check for corruption
+ * @key: pointer to the raw key
+ * @val: pointer to the raw value
+ * @len: length of the raw value
+ *
+ * Internal consistency of @key must be checked before calling this function.
+ */
+void parse_xattr_record(struct apfs_xattr_key *key,
+ struct apfs_xattr_val *val, int len)
+{
+ u16 flags;
+
+ if (len < sizeof(*val))
+ report("Xattr record", "value is too small.");
+ len -= sizeof(*val);
+
+ flags = le16_to_cpu(val->flags);
+
+ if (flags & APFS_XATTR_DATA_STREAM) {
+ if (len != sizeof(struct apfs_xattr_dstream))
+ report("Xattr record",
+ "bad length for dstream structure.");
+ if (len != le16_to_cpu(val->xdata_len))
+ /* Never seems to happen, but the docs don't ban it */
+ report_weird("Xattr data length for dstream structure");
+ } else {
+ if (len != le16_to_cpu(val->xdata_len))
+ report("Xattr record", "bad length for embedded data.");
+ }
+}
diff --git a/apfsck/xattr.h b/apfsck/xattr.h
new file mode 100644
index 0000000..ca18bf6
--- /dev/null
+++ b/apfsck/xattr.h
@@ -0,0 +1,50 @@
+/*
+ * apfsprogs/apfsck/xattr.h
+ *
+ * Copyright (C) 2018 Ernesto A. Fernández <ernesto.mn...@gmail.com>
+ */
+
+#ifndef _XATTR_H
+#define _XATTR_H
+
+#include "types.h"
+#include "inode.h"
+
+struct apfs_xattr_key;
+
+/* Extended attributes constants */
+#define APFS_XATTR_MAX_EMBEDDED_SIZE 3804
+
+/* Extended attributes names */
+#define APFS_XATTR_NAME_SYMLINK "com.apple.fs.symlink"
+#define APFS_XATTR_NAME_COMPRESSED "com.apple.decmpfs"
+
+/* Extended attributes flags */
+enum {
+ APFS_XATTR_DATA_STREAM = 0x00000001,
+ APFS_XATTR_DATA_EMBEDDED = 0x00000002,
+ APFS_XATTR_FILE_SYSTEM_OWNED = 0x00000004,
+ APFS_XATTR_RESERVED_8 = 0x00000008,
+};
+
+/*
+ * Structure of the value of an extended attributes record
+ */
+struct apfs_xattr_val {
+ __le16 flags;
+ __le16 xdata_len;
+ u8 xdata[0];
+} __packed;
+
+/*
+ * Structure used to store the data of an extended attributes record
+ */
+struct apfs_xattr_dstream {
+ __le64 xattr_obj_id;
+ struct apfs_dstream dstream;
+} __packed;
+
+extern void parse_xattr_record(struct apfs_xattr_key *key,
+ struct apfs_xattr_val *val, int len);
+
+#endif /* _XATTR_H */
--
2.11.0

[Ernesto A. Fernández]

Check that the flags of a xattr are all valid, and consistent with each
other.

Signed-off-by: Ernesto A. Fernández <ernesto.mn...@gmail.com>
---

apfsck/xattr.c | 17 +++++++++++++++++
apfsck/xattr.h | 2 ++
2 files changed, 19 insertions(+)

diff --git a/apfsck/xattr.c b/apfsck/xattr.c
index 1ec3627..0fec270 100644
--- a/apfsck/xattr.c
+++ b/apfsck/xattr.c
@@ -10,6 +10,22 @@
#include "xattr.h"

/**
+ * check_xattr_flags - Check basic consistency of xattr flags
+ * @flags: the flags
+ */
+static void check_xattr_flags(u16 flags)
+{
+ if ((flags & APFS_XATTR_VALID_FLAGS) != flags)
+ report("Xattr record", "invalid flags in use.");
+ if (flags & APFS_XATTR_RESERVED_8)
+ report("Xattr record", "reserved flag in use.");
+
+ if ((bool)(flags & APFS_XATTR_DATA_STREAM) ==
+ (bool)(flags & APFS_XATTR_DATA_EMBEDDED))
+ report("Xattr record", "must be either embedded or dstream.");
+}
+
+/**

* parse_xattr_record - Parse a xattr record value and check for corruption

* @key: pointer to the raw key

* @val: pointer to the raw value

@@ -27,6 +43,7 @@ void parse_xattr_record(struct apfs_xattr_key *key,
len -= sizeof(*val);

flags = le16_to_cpu(val->flags);
+ check_xattr_flags(flags);

if (flags & APFS_XATTR_DATA_STREAM) {

if (len != sizeof(struct apfs_xattr_dstream))

diff --git a/apfsck/xattr.h b/apfsck/xattr.h
index ca18bf6..f20b485 100644
--- a/apfsck/xattr.h
+++ b/apfsck/xattr.h
@@ -27,6 +27,8 @@ enum {
APFS_XATTR_RESERVED_8 = 0x00000008,
};

+#define APFS_XATTR_VALID_FLAGS 0x0000000f
+
/*

* Structure of the value of an extended attributes record

*/
--
2.11.0

[Ernesto A. Fernández]

A symlink inode must have a single special system-owned xattr for its
target, under the name "com.apple.fs.symlink". That xattr should never
be seen on files of other types. Add checks for this.

Signed-off-by: Ernesto A. Fernández <ernesto.mn...@gmail.com>
---

apfsck/inode.c | 5 +++++
apfsck/inode.h | 4 ++++
apfsck/xattr.c | 16 ++++++++++++++++
3 files changed, 25 insertions(+)

diff --git a/apfsck/inode.c b/apfsck/inode.c
index 27e0956..2da313d 100644
--- a/apfsck/inode.c
+++ b/apfsck/inode.c
@@ -43,6 +43,11 @@ static void check_inode_stats(struct inode *inode)
report("Inode record", "wrong allocated space for dstream.");
if (dstream->d_sparse_bytes != inode->i_sparse_bytes)
report("Inode record", "wrong count of sparse bytes.");
+
+ if ((bool)(inode->i_xattr_bmap & XATTR_BMAP_SYMLINK) !=
+ (bool)((inode->i_mode & S_IFMT) == S_IFLNK))
+ report("Inode record",
+ "symlink inode should come with target xattr.");
}

/**
diff --git a/apfsck/inode.h b/apfsck/inode.h
index ba70b8b..18199e2 100644
--- a/apfsck/inode.h
+++ b/apfsck/inode.h
@@ -177,6 +177,9 @@ struct apfs_sibling_val {

#define INODE_TABLE_BUCKETS 512 /* So the hash table array fits in 4k */

+/* Flags for the bitmap of seen system xattrs (i_xattr_bmap) */
+#define XATTR_BMAP_SYMLINK 0x01 /* Symlink target xattr */
+
/*
* Inode data in memory
*/
@@ -199,6 +202,7 @@ struct inode {
char *i_name; /* Name of primary link */

/* Inode stats measured by the fsck */
+ u8 i_xattr_bmap; /* Bitmap of system xattrs for inode */
u32 i_child_count; /* Number of children of directory */
u32 i_link_count; /* Number of dentries for file */
char *i_first_name; /* Name of first dentry encountered */
diff --git a/apfsck/xattr.c b/apfsck/xattr.c
index 0fec270..137ef70 100644
--- a/apfsck/xattr.c
+++ b/apfsck/xattr.c
@@ -4,8 +4,11 @@

* Copyright (C) 2018 Ernesto A. Fernández <ernesto.mn...@gmail.com>

*/

+#include <string.h>
#include "apfsck.h"
+#include "inode.h"
#include "key.h"
+#include "super.h"
#include "types.h"
#include "xattr.h"

@@ -36,6 +39,7 @@ static void check_xattr_flags(u16 flags)
void parse_xattr_record(struct apfs_xattr_key *key,

struct apfs_xattr_val *val, int len)
{

+ struct inode *inode;
u16 flags;

if (len < sizeof(*val))
@@ -56,4 +60,16 @@ void parse_xattr_record(struct apfs_xattr_key *key,

if (len != le16_to_cpu(val->xdata_len))

report("Xattr record", "bad length for embedded data.");
}
+

+ inode = get_inode(cat_cnid(&key->hdr), vsb->v_inode_table);
+ if (!inode->i_seen)
+ report("Catalog", "xattr record with no inode.");
+
+ if (!strcmp((char *)key->name, APFS_XATTR_NAME_SYMLINK)) {
+ if (!(flags & APFS_XATTR_FILE_SYSTEM_OWNED))
+ report("Symlink target xattr", "not owned by system.");
+ if (inode->i_xattr_bmap & XATTR_BMAP_SYMLINK)
+ report("Catalog", "two targets for same symlink.");
+ inode->i_xattr_bmap |= XATTR_BMAP_SYMLINK;
+ }
}
--
2.11.0

[Ernesto A. Fernández]

I was under the impression that resource forks would be tricky to check,
so the fsck tool is reporting their presence as an unsupported feature.
But it seems that resource forks are actually just another xattr, under
the name "com.apple.ResourceFork".

Check that all inodes with the HAS_RSRC_FORK flag set have this xattr,
and vice versa. Note that, unlike symlink targets, these xattrs are not
system-owned.

Signed-off-by: Ernesto A. Fernández <ernesto.mn...@gmail.com>
---

apfsck/inode.c | 5 +++--
apfsck/inode.h | 1 +
apfsck/xattr.c | 6 ++++++
apfsck/xattr.h | 1 +
4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/apfsck/inode.c b/apfsck/inode.c
index 2da313d..61f3b05 100644
--- a/apfsck/inode.c
+++ b/apfsck/inode.c
@@ -48,6 +48,9 @@ static void check_inode_stats(struct inode *inode)

(bool)((inode->i_mode & S_IFMT) == S_IFLNK))

report("Inode record",

"symlink inode should come with target xattr.");

+ if ((bool)(inode->i_xattr_bmap & XATTR_BMAP_RSRC_FORK) !=
+ (bool)(inode->i_flags & APFS_INODE_HAS_RSRC_FORK))
+ report("Inode record", "wrong flag for resource fork.");
}

/**
@@ -517,8 +520,6 @@ static void check_inode_internal_flags(u64 flags)
flags & APFS_INODE_PINNED_TO_TIER2 ||
flags & APFS_INODE_ALLOCATION_SPILLEDOVER)
report_unknown("Fusion drive");
- if (flags & APFS_INODE_HAS_RSRC_FORK)
- report_unknown("Resource fork");
if (flags & APFS_INODE_MAINTAIN_DIR_STATS)
report_unknown("Directory statistics");
if (flags & APFS_INODE_IS_APFS_PRIVATE)
diff --git a/apfsck/inode.h b/apfsck/inode.h
index 18199e2..f14b407 100644
--- a/apfsck/inode.h
+++ b/apfsck/inode.h
@@ -179,6 +179,7 @@ struct apfs_sibling_val {

/* Flags for the bitmap of seen system xattrs (i_xattr_bmap) */

#define XATTR_BMAP_SYMLINK 0x01 /* Symlink target xattr */

+#define XATTR_BMAP_RSRC_FORK 0x02 /* Resource fork xattr */

/*
* Inode data in memory

diff --git a/apfsck/xattr.c b/apfsck/xattr.c
index 137ef70..46017e3 100644
--- a/apfsck/xattr.c
+++ b/apfsck/xattr.c
@@ -71,5 +71,11 @@ void parse_xattr_record(struct apfs_xattr_key *key,
if (inode->i_xattr_bmap & XATTR_BMAP_SYMLINK)

report("Catalog", "two targets for same symlink.");

inode->i_xattr_bmap |= XATTR_BMAP_SYMLINK;
+ } else if (!strcmp((char *)key->name, APFS_XATTR_NAME_RSRC_FORK)) {
+ if (flags & APFS_XATTR_FILE_SYSTEM_OWNED)
+ report("Resource fork xattr", "owned by system.");
+ if (inode->i_xattr_bmap & XATTR_BMAP_RSRC_FORK)
+ report("Catalog", "two resource forks for same inode.");
+ inode->i_xattr_bmap |= XATTR_BMAP_RSRC_FORK;
}
}
diff --git a/apfsck/xattr.h b/apfsck/xattr.h
index f20b485..7d0562e 100644
--- a/apfsck/xattr.h
+++ b/apfsck/xattr.h
@@ -18,6 +18,7 @@ struct apfs_xattr_key;
/* Extended attributes names */
#define APFS_XATTR_NAME_SYMLINK "com.apple.fs.symlink"
#define APFS_XATTR_NAME_COMPRESSED "com.apple.decmpfs"
+#define APFS_XATTR_NAME_RSRC_FORK "com.apple.ResourceFork"

/* Extended attributes flags */
enum {
--
2.11.0

[Ernesto A. Fernández]

Verify that inodes with the HAS_SECURITY_EA flag actually have a xattr
for the access control lists, and that no other inode does.

Somewhat surprisingly, these xattrs are not system-owned.

Signed-off-by: Ernesto A. Fernández <ernesto.mn...@gmail.com>
---

apfsck/inode.c | 3 +++

apfsck/inode.h | 1 +
apfsck/xattr.c | 6 ++++++
apfsck/xattr.h | 1 +
4 files changed, 11 insertions(+)

diff --git a/apfsck/inode.c b/apfsck/inode.c
index 61f3b05..a5cfbac 100644
--- a/apfsck/inode.c
+++ b/apfsck/inode.c
@@ -51,6 +51,9 @@ static void check_inode_stats(struct inode *inode)

if ((bool)(inode->i_xattr_bmap & XATTR_BMAP_RSRC_FORK) !=

(bool)(inode->i_flags & APFS_INODE_HAS_RSRC_FORK))

report("Inode record", "wrong flag for resource fork.");

+ if ((bool)(inode->i_xattr_bmap & XATTR_BMAP_SECURITY) !=
+ (bool)(inode->i_flags & APFS_INODE_HAS_SECURITY_EA))
+ report("Inode record", "wrong flag for access control list.");
}

/**
diff --git a/apfsck/inode.h b/apfsck/inode.h
index f14b407..c78c36e 100644
--- a/apfsck/inode.h
+++ b/apfsck/inode.h
@@ -180,6 +180,7 @@ struct apfs_sibling_val {

/* Flags for the bitmap of seen system xattrs (i_xattr_bmap) */
#define XATTR_BMAP_SYMLINK 0x01 /* Symlink target xattr */

#define XATTR_BMAP_RSRC_FORK 0x02 /* Resource fork xattr */

+#define XATTR_BMAP_SECURITY 0x04 /* Security xattr */

/*
* Inode data in memory
diff --git a/apfsck/xattr.c b/apfsck/xattr.c

index 46017e3..f509ef3 100644
--- a/apfsck/xattr.c
+++ b/apfsck/xattr.c
@@ -77,5 +77,11 @@ void parse_xattr_record(struct apfs_xattr_key *key,
if (inode->i_xattr_bmap & XATTR_BMAP_RSRC_FORK)

report("Catalog", "two resource forks for same inode.");

inode->i_xattr_bmap |= XATTR_BMAP_RSRC_FORK;
+ } else if (!strcmp((char *)key->name, APFS_XATTR_NAME_SECURITY)) {

+ if (flags & APFS_XATTR_FILE_SYSTEM_OWNED)

+ report("Security xattr", "owned by system.");
+ if (inode->i_xattr_bmap & XATTR_BMAP_SECURITY)
+ report("Catalog", "two security xattrs for one inode.");
+ inode->i_xattr_bmap |= XATTR_BMAP_SECURITY;
}
}
diff --git a/apfsck/xattr.h b/apfsck/xattr.h
index 7d0562e..125ecdd 100644
--- a/apfsck/xattr.h
+++ b/apfsck/xattr.h
@@ -19,6 +19,7 @@ struct apfs_xattr_key;

#define APFS_XATTR_NAME_SYMLINK "com.apple.fs.symlink"
#define APFS_XATTR_NAME_COMPRESSED "com.apple.decmpfs"

#define APFS_XATTR_NAME_RSRC_FORK "com.apple.ResourceFork"
+#define APFS_XATTR_NAME_SECURITY "com.apple.system.Security"