Re: [Qemu-devel] [PATCH 1/2] qed: check for header size overflow

[Kevin Wolf <kwolf@redhat.com>]

Am 12.01.2015 um 13:31 hat Stefan Hajnoczi geschrieben:
> Header size is denoted in clusters.  The maximum cluster size is 64 MB
> but there is no limit on header size.  Check for uint32_t overflow in
> case the header size field has a whacky value.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/qed.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 80f18d8..d2c6045 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -440,6 +440,12 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
>      s->l2_mask = s->table_nelems - 1;
>      s->l1_shift = s->l2_shift + ffs(s->table_nelems) - 1;
>  
> +    /* Header size calculation must not overflow uint32_t */
> +    if ((uint64_t)s->header.cluster_size * s->header.header_size !=
> +        s->header.cluster_size * s->header.header_size) {

Generally, I find checks that don't exercise the integer overflow easier
to read, i.e. in this case:

    if (s->header.header_size > UINT32_MAX / s->header.cluster_size)

Or even just INT_MAX to be on the safe side. But I'll admit that it's a
matter of taste.

> +        return -EINVAL;
> +    }
> +
>      if ((s->header.features & QED_F_BACKING_FILE)) {
>          if ((uint64_t)s->header.backing_filename_offset +
>              s->header.backing_filename_size >

Series: Reviewed-by: Kevin Wolf <kwolf@redhat.com>