Skip to content

Commit

Permalink
zfs_log: add flex array fields to log record structs
Browse files Browse the repository at this point in the history
ZIL log record structs (lr_XX_t) are frequently allocated with extra
space after the struct to carry variable-sized "payload" items.

Linux 6.10+ compiled with CONFIG_FORTIFY_SOURCE has been doing runtime
bounds checking on memcpy() calls. Because these types had no indicator
that they might use more space than their simple definition,
__fortify_memcpy_chk will frequently complain about overruns eg:

    memcpy: detected field-spanning write (size 7) of single field
        "lr + 1" at zfs_log.c:425 (size 0)
    memcpy: detected field-spanning write (size 9) of single field
        "(char *)(lr + 1)" at zfs_log.c:593 (size 0)
    memcpy: detected field-spanning write (size 4) of single field
        "(char *)(lr + 1) + snamesize" at zfs_log.c:594 (size 0)
    memcpy: detected field-spanning write (size 7) of single field
        "lr + 1" at zfs_log.c:425 (size 0)
    memcpy: detected field-spanning write (size 9) of single field
        "(char *)(lr + 1)" at zfs_log.c:593 (size 0)
    memcpy: detected field-spanning write (size 4) of single field
        "(char *)(lr + 1) + snamesize" at zfs_log.c:594 (size 0)
    memcpy: detected field-spanning write (size 7) of single field
        "lr + 1" at zfs_log.c:425 (size 0)
    memcpy: detected field-spanning write (size 9) of single field
        "(char *)(lr + 1)" at zfs_log.c:593 (size 0)
    memcpy: detected field-spanning write (size 4) of single field
        "(char *)(lr + 1) + snamesize" at zfs_log.c:594 (size 0)

To fix this, this commit adds flex array fields to all lr_XX_t structs
that require them, and then uses those fields to access that
end-of-struct area rather than more complicated casts and pointer
addition.

Sponsored-by: https://despairlabs.com/sponsor/
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rob Norris <robn@despairlabs.com>
Closes openzfs#16501
Closes openzfs#16539
  • Loading branch information
robn authored Sep 27, 2024
1 parent ac8837d commit 6f50f8e
Show file tree
Hide file tree
Showing 5 changed files with 162 additions and 131 deletions.
6 changes: 4 additions & 2 deletions cmd/zdb/zdb_il.c
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ static void
zil_prt_rec_create(zilog_t *zilog, int txtype, const void *arg)
{
(void) zilog;
const lr_create_t *lr = arg;
const lr_create_t *lrc = arg;
const _lr_create_t *lr = &lrc->lr_create;
time_t crtime = lr->lr_crtime[0];
char *name, *link;
lr_attr_t *lrattr;
Expand Down Expand Up @@ -121,7 +122,8 @@ static void
zil_prt_rec_rename(zilog_t *zilog, int txtype, const void *arg)
{
(void) zilog, (void) txtype;
const lr_rename_t *lr = arg;
const lr_rename_t *lrr = arg;
const _lr_rename_t *lr = &lrr->lr_rename;
char *snm = (char *)(lr + 1);
char *tnm = snm + strlen(snm) + 1;

Expand Down
24 changes: 13 additions & 11 deletions cmd/ztest.c
Original file line number Diff line number Diff line change
Expand Up @@ -1940,15 +1940,15 @@ ztest_verify_unused_bonus(dmu_buf_t *db, void *end, uint64_t obj,
static void
ztest_log_create(ztest_ds_t *zd, dmu_tx_t *tx, lr_create_t *lr)
{
char *name = (void *)(lr + 1); /* name follows lr */
char *name = (char *)&lr->lr_data[0]; /* name follows lr */
size_t namesize = strlen(name) + 1;
itx_t *itx;

if (zil_replaying(zd->zd_zilog, tx))
return;

itx = zil_itx_create(TX_CREATE, sizeof (*lr) + namesize);
memcpy(&itx->itx_lr + 1, &lr->lr_common + 1,
memcpy(&itx->itx_lr + 1, &lr->lr_create.lr_common + 1,
sizeof (*lr) + namesize - sizeof (lr_t));

zil_itx_assign(zd->zd_zilog, itx, tx);
Expand All @@ -1957,7 +1957,7 @@ ztest_log_create(ztest_ds_t *zd, dmu_tx_t *tx, lr_create_t *lr)
static void
ztest_log_remove(ztest_ds_t *zd, dmu_tx_t *tx, lr_remove_t *lr, uint64_t object)
{
char *name = (void *)(lr + 1); /* name follows lr */
char *name = (char *)&lr->lr_data[0]; /* name follows lr */
size_t namesize = strlen(name) + 1;
itx_t *itx;

Expand Down Expand Up @@ -2043,8 +2043,9 @@ static int
ztest_replay_create(void *arg1, void *arg2, boolean_t byteswap)
{
ztest_ds_t *zd = arg1;
lr_create_t *lr = arg2;
char *name = (void *)(lr + 1); /* name follows lr */
lr_create_t *lrc = arg2;
_lr_create_t *lr = &lrc->lr_create;
char *name = (char *)&lrc->lr_data[0]; /* name follows lr */
objset_t *os = zd->zd_os;
ztest_block_tag_t *bbt;
dmu_buf_t *db;
Expand Down Expand Up @@ -2122,7 +2123,7 @@ ztest_replay_create(void *arg1, void *arg2, boolean_t byteswap)
VERIFY0(zap_add(os, lr->lr_doid, name, sizeof (uint64_t), 1,
&lr->lr_foid, tx));

(void) ztest_log_create(zd, tx, lr);
(void) ztest_log_create(zd, tx, lrc);

dmu_tx_commit(tx);

Expand All @@ -2134,7 +2135,7 @@ ztest_replay_remove(void *arg1, void *arg2, boolean_t byteswap)
{
ztest_ds_t *zd = arg1;
lr_remove_t *lr = arg2;
char *name = (void *)(lr + 1); /* name follows lr */
char *name = (char *)&lr->lr_data[0]; /* name follows lr */
objset_t *os = zd->zd_os;
dmu_object_info_t doi;
dmu_tx_t *tx;
Expand Down Expand Up @@ -2188,9 +2189,9 @@ ztest_replay_write(void *arg1, void *arg2, boolean_t byteswap)
ztest_ds_t *zd = arg1;
lr_write_t *lr = arg2;
objset_t *os = zd->zd_os;
void *data = lr + 1; /* data follows lr */
uint8_t *data = &lr->lr_data[0]; /* data follows lr */
uint64_t offset, length;
ztest_block_tag_t *bt = data;
ztest_block_tag_t *bt = (ztest_block_tag_t *)data;
ztest_block_tag_t *bbt;
uint64_t gen, txg, lrtxg, crtxg;
dmu_object_info_t doi;
Expand Down Expand Up @@ -2649,7 +2650,8 @@ ztest_create(ztest_ds_t *zd, ztest_od_t *od, int count)
continue;
}

lr_create_t *lr = ztest_lr_alloc(sizeof (*lr), od->od_name);
lr_create_t *lrc = ztest_lr_alloc(sizeof (*lrc), od->od_name);
_lr_create_t *lr = &lrc->lr_create;

lr->lr_doid = od->od_dir;
lr->lr_foid = 0; /* 0 to allocate, > 0 to claim */
Expand Down Expand Up @@ -2733,7 +2735,7 @@ ztest_write(ztest_ds_t *zd, uint64_t object, uint64_t offset, uint64_t size,
lr->lr_blkoff = 0;
BP_ZERO(&lr->lr_blkptr);

memcpy(lr + 1, data, size);
memcpy(&lr->lr_data[0], data, size);

error = ztest_replay_write(zd, lr, B_FALSE);

Expand Down
30 changes: 25 additions & 5 deletions include/sys/zil.h
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ typedef struct {
uint32_t lr_attr_masksize; /* number of elements in array */
uint32_t lr_attr_bitmap; /* First entry of array */
/* remainder of array and additional lr_attr_end_t fields */
uint8_t lr_attr_data[];
} lr_attr_t;

/*
Expand All @@ -264,9 +265,14 @@ typedef struct {
uint64_t lr_gen; /* generation (txg of creation) */
uint64_t lr_crtime[2]; /* creation time */
uint64_t lr_rdev; /* rdev of object to create */
} _lr_create_t;

typedef struct {
_lr_create_t lr_create; /* common create portion */
/* name of object to create follows this */
/* for symlinks, link content follows name */
/* for creates with xvattr data, the name follows the xvattr info */
uint8_t lr_data[];
} lr_create_t;

/*
Expand All @@ -293,37 +299,45 @@ typedef struct {
* and group will be in lr_create. Name follows ACL data.
*/
typedef struct {
lr_create_t lr_create; /* common create portion */
_lr_create_t lr_create; /* common create portion */
uint64_t lr_aclcnt; /* number of ACEs in ACL */
uint64_t lr_domcnt; /* number of unique domains */
uint64_t lr_fuidcnt; /* number of real fuids */
uint64_t lr_acl_bytes; /* number of bytes in ACL */
uint64_t lr_acl_flags; /* ACL flags */
uint8_t lr_data[];
} lr_acl_create_t;

typedef struct {
lr_t lr_common; /* common portion of log record */
uint64_t lr_doid; /* obj id of directory */
/* name of object to remove follows this */
uint8_t lr_data[];
} lr_remove_t;

typedef struct {
lr_t lr_common; /* common portion of log record */
uint64_t lr_doid; /* obj id of directory */
uint64_t lr_link_obj; /* obj id of link */
/* name of object to link follows this */
uint8_t lr_data[];
} lr_link_t;

typedef struct {
lr_t lr_common; /* common portion of log record */
uint64_t lr_sdoid; /* obj id of source directory */
uint64_t lr_tdoid; /* obj id of target directory */
} _lr_rename_t;

typedef struct {
_lr_rename_t lr_rename; /* common rename portion */
/* 2 strings: names of source and destination follow this */
uint8_t lr_data[];
} lr_rename_t;

typedef struct {
lr_rename_t lr_rename; /* common rename portion */
/* members related to the whiteout file (based on lr_create_t) */
_lr_rename_t lr_rename; /* common rename portion */
/* members related to the whiteout file (based on _lr_create_t) */
uint64_t lr_wfoid; /* obj id of the new whiteout file */
uint64_t lr_wmode; /* mode of object */
uint64_t lr_wuid; /* uid of whiteout */
Expand All @@ -332,6 +346,7 @@ typedef struct {
uint64_t lr_wcrtime[2]; /* creation time */
uint64_t lr_wrdev; /* always makedev(0, 0) */
/* 2 strings: names of source and destination follow this */
uint8_t lr_data[];
} lr_rename_whiteout_t;

typedef struct {
Expand All @@ -342,6 +357,7 @@ typedef struct {
uint64_t lr_blkoff; /* no longer used */
blkptr_t lr_blkptr; /* spa block pointer for replay */
/* write data will follow for small writes */
uint8_t lr_data[];
} lr_write_t;

typedef struct {
Expand All @@ -362,20 +378,23 @@ typedef struct {
uint64_t lr_atime[2]; /* access time */
uint64_t lr_mtime[2]; /* modification time */
/* optional attribute lr_attr_t may be here */
uint8_t lr_data[];
} lr_setattr_t;

typedef struct {
lr_t lr_common; /* common portion of log record */
uint64_t lr_foid; /* file object to change attributes */
uint64_t lr_size;
/* xattr name and value follows */
uint8_t lr_data[];
} lr_setsaxattr_t;

typedef struct {
lr_t lr_common; /* common portion of log record */
uint64_t lr_foid; /* obj id of file */
uint64_t lr_aclcnt; /* number of acl entries */
/* lr_aclcnt number of ace_t entries follow this */
uint8_t lr_data[];
} lr_acl_v0_t;

typedef struct {
Expand All @@ -387,6 +406,7 @@ typedef struct {
uint64_t lr_acl_bytes; /* number of bytes in ACL */
uint64_t lr_acl_flags; /* ACL flags */
/* lr_acl_bytes number of variable sized ace's follows */
uint8_t lr_data[];
} lr_acl_t;

typedef struct {
Expand All @@ -396,8 +416,8 @@ typedef struct {
uint64_t lr_length; /* length of the blocks to clone */
uint64_t lr_blksz; /* file's block size */
uint64_t lr_nbps; /* number of block pointers */
blkptr_t lr_bps[];
/* block pointers of the blocks to clone follows */
blkptr_t lr_bps[];
} lr_clone_range_t;

/*
Expand Down Expand Up @@ -448,7 +468,7 @@ typedef struct itx {
uint64_t itx_oid; /* object id */
uint64_t itx_gen; /* gen number for zfs_get_data */
lr_t itx_lr; /* common part of log record */
/* followed by type-specific part of lr_xx_t and its immediate data */
uint8_t itx_lr_data[]; /* type-specific part of lr_xx_t */
} itx_t;

/*
Expand Down
Loading

0 comments on commit 6f50f8e

Please sign in to comment.