Re: [PATCH] jfs: Avoid field-overflowing memcpy()

[Dave Kleikamp]

On 6/21/21 6:23 PM, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field array bounds checking for memcpy(), memmove(), and memset(),
> avoid intentionally writing across neighboring fields.
>
> Introduce more unions to cover the full inline data section, so that the
> entire 256 bytes can be addressed by memcpy() without thinking it is
> crossing field boundaries. Additionally adjusts dir memcpy() to use
> existing union names to get the same coverage.

This all makes sense and looks reasonable. I'll push it to -next.

Thanks,
Shaggy

> diffoscope shows there are no binary differences before/after excepting
> the name of the initcall, which is line number based:
>
> $ diffoscope --exclude-directory-metadata yes before/fs after/fs
> --- before/fs
> +++ after/fs
> │   --- before/fs/jfs
> ├── +++ after/fs/jfs
> │ │   --- before/fs/jfs/super.o
> │ ├── +++ after/fs/jfs/super.o
> │ │ ├── readelf --wide --symbols {}
> │ │ │ @@ -2,15 +2,15 @@
> │ │ │  Symbol table '.symtab' contains 158 entries:
> │ │ │     Num:    Value          Size Type    Bind   Vis      Ndx Name
> ...
> │ │ │ -     5: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT    6 __initcall__kmod_jfs__319_1049_ini
> t_jfs_fs6
> │ │ │ +     5: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT    6 __initcall__kmod_jfs__319_1050_ini
> t_jfs_fs6
> ...
>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>   fs/jfs/jfs_dinode.h | 14 ++++++++++----
>   fs/jfs/jfs_imap.c   |  4 ++--
>   fs/jfs/jfs_incore.h | 12 ++++++++++--
>   fs/jfs/super.c      |  3 ++-
>   4 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/fs/jfs/jfs_dinode.h b/fs/jfs/jfs_dinode.h
> index d6af79e94263..6b231d0d0071 100644
> --- a/fs/jfs/jfs_dinode.h
> +++ b/fs/jfs/jfs_dinode.h
> @@ -101,7 +101,6 @@ struct dinode {
>   					u8 unused[16];	/* 16: */
>   					dxd_t _dxd;	/* 16: */
>   					union {
> -						__le32 _rdev;	/* 4: */
>   						/*
>   						 * The fast symlink area
>   						 * is expected to overflow
> @@ -109,9 +108,15 @@ struct dinode {
>   						 * needed (which will clear
>   						 * INLINEEA).
>   						 */
> -						u8 _fastsymlink[128];
> -					} _u;
> -					u8 _inlineea[128];
> +						struct {
> +							union {
> +								__le32 _rdev;	/* 4: */
> +								u8 _fastsymlink[128];
> +							} _u;
> +							u8 _inlineea[128];
> +						};
> +						u8 _inline_all[256];
> +					};
>   				} _special;
>   			} _u2;
>   		} _file;
> @@ -122,6 +127,7 @@ struct dinode {
>   #define di_rdev		u._file._u2._special._u._rdev
>   #define di_fastsymlink	u._file._u2._special._u._fastsymlink
>   #define di_inlineea	u._file._u2._special._inlineea
> +#define di_inline_all	u._file._u2._special._inline_all
>   	} u;
>   };
>   
> diff --git a/fs/jfs/jfs_imap.c b/fs/jfs/jfs_imap.c
> index 937ca07b58b1..4df3f222c35c 100644
> --- a/fs/jfs/jfs_imap.c
> +++ b/fs/jfs/jfs_imap.c
> @@ -763,7 +763,7 @@ int diWrite(tid_t tid, struct inode *ip)
>   		lv = & dilinelock->lv[dilinelock->index];
>   		lv->offset = (dioffset + 2 * 128) >> L2INODESLOTSIZE;
>   		lv->length = 2;
> -		memcpy(&dp->di_fastsymlink, jfs_ip->i_inline, IDATASIZE);
> +		memcpy(&dp->di_inline_all, jfs_ip->i_inline_all, IDATASIZE);
>   		dilinelock->index++;
>   	}
>   	/*
> @@ -3084,7 +3084,7 @@ static int copy_from_dinode(struct dinode * dip, struct inode *ip)
>   	}
>   
>   	if (S_ISDIR(ip->i_mode)) {
> -		memcpy(&jfs_ip->i_dirtable, &dip->di_dirtable, 384);
> +		memcpy(&jfs_ip->u.dir, &dip->u._dir, 384);
>   	} else if (S_ISREG(ip->i_mode) || S_ISLNK(ip->i_mode)) {
>   		memcpy(&jfs_ip->i_xtroot, &dip->di_xtroot, 288);
>   	} else
> diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h
> index a466ec41cfbb..721def69e732 100644
> --- a/fs/jfs/jfs_incore.h
> +++ b/fs/jfs/jfs_incore.h
> @@ -77,11 +77,18 @@ struct jfs_inode_info {
>   			unchar _unused[16];	/* 16: */
>   			dxd_t _dxd;		/* 16: */
>   			/* _inline may overflow into _inline_ea when needed */
> -			unchar _inline[128];	/* 128: inline symlink */
>   			/* _inline_ea may overlay the last part of
>   			 * file._xtroot if maxentry = XTROOTINITSLOT
>   			 */
> -			unchar _inline_ea[128];	/* 128: inline extended attr */
> +			union {
> +				struct {
> +					/* 128: inline symlink */
> +					unchar _inline[128];
> +					/* 128: inline extended attr */
> +					unchar _inline_ea[128];
> +				};
> +				unchar _inline_all[256];
> +			};
>   		} link;
>   	} u;
>   #ifdef CONFIG_QUOTA
> @@ -96,6 +103,7 @@ struct jfs_inode_info {
>   #define i_dtroot u.dir._dtroot
>   #define i_inline u.link._inline
>   #define i_inline_ea u.link._inline_ea
> +#define i_inline_all u.link._inline_all
>   
>   #define IREAD_LOCK(ip, subclass) \
>   	down_read_nested(&JFS_IP(ip)->rdwrlock, subclass)
> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> index 1f0ffabbde56..9030aeaf0f88 100644
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c
> @@ -939,7 +939,8 @@ static int __init init_jfs_fs(void)
>   	jfs_inode_cachep =
>   	    kmem_cache_create_usercopy("jfs_ip", sizeof(struct jfs_inode_info),
>   			0, SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD|SLAB_ACCOUNT,
> -			offsetof(struct jfs_inode_info, i_inline), IDATASIZE,
> +			offsetof(struct jfs_inode_info, i_inline_all),
> +			sizeof_field(struct jfs_inode_info, i_inline_all),
>   			init_once);
>   	if (jfs_inode_cachep == NULL)
>   		return -ENOMEM;