On Sun, Jan 26, 2020 at 12:35:39PM +0100, Christoph Hellwig wrote:
> Add a little helper to validate the nex count so that we can use compile
> time magic checks for sizeof long directly. Also don't print the max
> in case of an overflow as the value will always be the same.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> repair/bmap.c | 29 +++++++++++++++++++++--------
> repair/bmap.h | 13 -------------
> 2 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/repair/bmap.c b/repair/bmap.c
> index 44e43ab4..0d717cd3 100644
> --- a/repair/bmap.c
> +++ b/repair/bmap.c
> @@ -22,6 +22,22 @@
> pthread_key_t dblkmap_key;
> pthread_key_t ablkmap_key;
>
> +/*
> + * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
> + * limits the number of extents in an inode we can check. If we don't limit the
> + * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
> + * memory than we think we needed, and hence walk off the end of the array and
> + * corrupt memory.
> + */
> +static inline bool
> +blkmap_nex_valid(
> + xfs_extnum_t nex)
> +{
> + if (sizeof(long) < 64 && nex >= INT_MAX / sizeof(bmap_ext_t))
sizeof(long) < 8
Frankly I suspect the maximum array length on 32 bit platforms is far
less than 2^31 bytes...
--D
> + return false;
> + return true;
> +}
> +
> blkmap_t *
> blkmap_alloc(
> xfs_extnum_t nex,
> @@ -35,8 +51,7 @@ blkmap_alloc(
> if (nex < 1)
> nex = 1;
>
> -#if (BITS_PER_LONG == 32) /* on 64-bit platforms this is never true */
> - if (nex > BLKMAP_NEXTS_MAX) {
> + if (!blkmap_nex_valid(nex)) {
> do_warn(
> _("Number of extents requested in blkmap_alloc (%d) overflows 32 bits.\n"
> "If this is not a corruption, then you will need a 64 bit system\n"
> @@ -44,7 +59,6 @@ blkmap_alloc(
> nex);
> return NULL;
> }
> -#endif
>
> key = whichfork ? ablkmap_key : dblkmap_key;
> blkmap = pthread_getspecific(key);
> @@ -278,20 +292,19 @@ blkmap_grow(
> ASSERT(pthread_getspecific(key) == blkmap);
> }
>
> -#if (BITS_PER_LONG == 32) /* on 64-bit platforms this is never true */
> - if (new_naexts > BLKMAP_NEXTS_MAX) {
> + if (!blkmap_nex_valid(new_naexts)) {
> do_error(
> _("Number of extents requested in blkmap_grow (%d) overflows 32 bits.\n"
> "You need a 64 bit system to repair this filesystem.\n"),
> new_naexts);
> return NULL;
> }
> -#endif
> +
> if (new_naexts <= 0) {
> do_error(
> _("Number of extents requested in blkmap_grow (%d) overflowed the\n"
> - "maximum number of supported extents (%d).\n"),
> - new_naexts, BLKMAP_NEXTS_MAX);
> + "maximum number of supported extents.\n"),
> + new_naexts);
> return NULL;
> }
>
> diff --git a/repair/bmap.h b/repair/bmap.h
> index 4b588df8..df9602b3 100644
> --- a/repair/bmap.h
> +++ b/repair/bmap.h
> @@ -28,19 +28,6 @@ typedef struct blkmap {
> #define BLKMAP_SIZE(n) \
> (offsetof(blkmap_t, exts) + (sizeof(bmap_ext_t) * (n)))
>
> -/*
> - * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
> - * limits the number of extents in an inode we can check. If we don't limit the
> - * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
> - * memory than we think we needed, and hence walk off the end of the array and
> - * corrupt memory.
> - */
> -#if BITS_PER_LONG == 32
> -#define BLKMAP_NEXTS_MAX ((INT_MAX / sizeof(bmap_ext_t)) - 1)
> -#else
> -#define BLKMAP_NEXTS_MAX INT_MAX
> -#endif
> -
> extern pthread_key_t dblkmap_key;
> extern pthread_key_t ablkmap_key;
>
> --
> 2.24.1
>
