Re: [PATCH 3/5] tmem: Check copy_to_user_* return value.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:
> We weren't checking whether that operation fails and
> return the proper error.
>
> This fixes CID 1055125, 105512, 1055127, 1055128, 1055129,
> 1055130.
>
> CC: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/common/tmem.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index 081772e..3bc35fd 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len,
>      if ( cli_id == TMEM_CLI_ID_NULL ) {
>          off = tmemc_list_global(buf,0,len,use_long);
>          off += tmemc_list_shared(buf,off,len-off,use_long);
> -        list_for_each_entry(client,&global_client_list,client_list)
> -            off += tmemc_list_client(client, buf, off, len-off, use_long);
> +        list_for_each_entry(client,&global_client_list,client_list) {

Spaces and commas.

> +            int ret = tmemc_list_client(client, buf, off, len-off, use_long);
> +            if ( ret < 0 )
> +                return ret;
> +            off += ret;
> +        }
>          off += tmemc_list_global_perf(buf,off,len-off,use_long);
>      }
>      else if ( (client = tmem_client_from_cli_id(cli_id)) == NULL)
> @@ -2155,6 +2159,8 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len,
>      else
>          off = tmemc_list_client(client, buf, 0, len, use_long);
>  
> +    if ( off < 0 )
> +        return off;

This looks to check for an overflow of 'off', but it is too late. 
Overflow needs to be checked each time you possibly add more to it.

~Andrew

>      return 0;
>  }
>  
> @@ -2319,8 +2325,9 @@ static int tmemc_save_subop(int cli_id, uint32_t pool_id,
>      case TMEMC_SAVE_GET_POOL_UUID:
>           if ( pool == NULL )
>               break;
> -        tmem_copy_to_client_buf(buf, pool->uuid, 2);
>          rc = 0;
> +        if ( tmem_copy_to_client_buf(buf, pool->uuid, 2) )
> +            rc = -EFAULT;
>          break;
>      case TMEMC_SAVE_END:
>          if ( client == NULL )
> @@ -2383,7 +2390,10 @@ static int tmemc_save_get_next_page(int cli_id, uint32_t pool_id,
>      BUILD_BUG_ON(sizeof(h.oid) != sizeof(oid));
>      memcpy(h.oid, oid.oid, sizeof(h.oid));
>      h.index = pgp->index;
> -    tmem_copy_to_client_buf(buf, &h, 1);
> +    if ( tmem_copy_to_client_buf(buf, &h, 1) ) {
> +        ret = -EFAULT;
> +        goto out;
> +    }
>      tmem_client_buf_add(buf, sizeof(h));
>      ret = do_tmem_get(pool, &oid, pgp->index, 0, 0, 0, pagesize, buf);
>  
> @@ -2427,8 +2437,9 @@ static int tmemc_save_get_next_inv(int cli_id, tmem_cli_va_param_t buf,
>      BUILD_BUG_ON(sizeof(h.oid) != sizeof(pgp->inv_oid));
>      memcpy(h.oid, pgp->inv_oid.oid, sizeof(h.oid));
>      h.index = pgp->index;
> -    tmem_copy_to_client_buf(buf, &h, 1);
>      ret = 1;
> +    if ( tmem_copy_to_client_buf(buf, &h, 1) )
> +        ret = -EFAULT;
>  out:
>      tmem_spin_unlock(&pers_lists_spinlock);
>      return ret;