2022-10-24 19:00:36

by Kees Cook

Subject: [PATCH 2/2] xfs: Use flex_cpy() to check extent copying

Use flex_cpy() instead of memcpy() to copy the bui_fmt flexible array
structure, which will perform bounds checking internally. Avoids the
false positive warning seen under FORTIFY_SOURCE:

memcpy: detected field-spanning write (size 48) of single field "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16)

Reported-by: Zorro Lang <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216563
Cc: "Darrick J. Wong" <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
fs/xfs/xfs_bmap_item.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 51f66e982484..5f135893df66 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -24,6 +24,7 @@
#include "xfs_error.h"
#include "xfs_log_priv.h"
#include "xfs_log_recover.h"
+#include <linux/flex_array.h>

struct kmem_cache *xfs_bui_cache;
struct kmem_cache *xfs_bud_cache;
@@ -624,10 +625,10 @@ xfs_bui_copy_format(
src_bui_fmt = buf->i_addr;
len = xfs_bui_log_format_sizeof(src_bui_fmt->bui_nextents);

- if (buf->i_len == len) {
- memcpy(dst_bui_fmt, src_bui_fmt, len);
+ if (buf->i_len == len &&
+ __flex_cpy(dst_bui_fmt, src_bui_fmt,
+ bui_extents, bui_nextents, __passthru) == 0)
return 0;
- }
XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
return -EFSCORRUPTED;
}
--
2.34.1

2022-10-25 09:25:02

by Andy Shevchenko

Subject: Re: [PATCH 2/2] xfs: Use flex_cpy() to check extent copying

On Mon, Oct 24, 2022 at 10:20:58AM -0700, Kees Cook wrote:
> Use flex_cpy() instead of memcpy() to copy the bui_fmt flexible array
> structure, which will perform bounds checking internally. Avoids the
> false positive warning seen under FORTIFY_SOURCE:
>
> memcpy: detected field-spanning write (size 48) of single field "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16)

...

> #include "xfs_error.h"
> #include "xfs_log_priv.h"
> #include "xfs_log_recover.h"

> +#include <linux/flex_array.h>

Wouldn't it be better to include it before any "local" headers?

--
With Best Regards,
Andy Shevchenko