Re: [PATCH 02/11] xfsprogs: fix integer overflow in xlog_find_verify_cycle

[Dave Chinner <david@fromorbit.com>]

On Wed, Dec 02, 2015 at 04:49:18PM +0530, Vivek Trivedi wrote:
> Fix unintentional integer overflow in xlog_find_verify_cycle.
> Reported by coverity.
> 
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
>  libxlog/xfs_log_recover.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
> index ef7cf68..73f4d36 100644
> --- a/libxlog/xfs_log_recover.c
> +++ b/libxlog/xfs_log_recover.c
> @@ -254,7 +254,7 @@ xlog_find_verify_cycle(
>  	 * try a smaller size.  We need to be able to read at least
>  	 * a log sector, or we're out of luck.
>  	 */
> -	bufblks = 1 << ffs(nbblks);
> +	bufblks = (xfs_daddr_t)1 << ffs(nbblks);

Ummm, in isolation that change is technically correct, but when you
look at what bufblks contains it is clearly wrong.  nbblks is an
int, so "1 << ffs(nbblks)" should not be larger than an int.

i.e. bufblks is simply a count of blocks in the log, which by
definition cannot be more than an int (in fact, 2^31 / 2^9 is the
largest legal value it can have). Hence it can't be larger than an
int, and all the functions it is passed to expect it to be an
int...

Hence the use of xfs_daddr_t is wrong, and that's the first thing
that needs fixing....

>  	while (bufblks > log->l_logBBsize)
>  		bufblks >>= 1;
>  	while (!(bp = xlog_get_bp(log, bufblks))) {
                                       ^^^^^^^^
And given this, it's clear that coverity doesn't warn about
overflows caused by passing a 64 bit value into a 32 bit function
parameter....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs