Re: [RFC PATCH v3 08/18] xen/arm: vITS: Add virtual ITS driver

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-06-22 at 17:31 +0530, vijay.kilari@gmail.com wrote:

> +    if ( !p2m_is_ram(p2mt) )
> +    {
> +        put_page(page);
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d with wrong attributes\n",
> +                d->domain_id, current->vcpu_id);

"d%dv%d" should be done (everywhere) with the "%pv" format code passing
current as the argument. See docs/misc/printk-formats.txt.

> +        return -EINVAL;
> +        return -EINVAL;
> +    }
> +
> +    p = __map_domain_page(page);
> +    /* Offset within the mapped page */
> +    offset = dt_entry & (PAGE_SIZE - 1);

This should be "& ~PAGE_MASK" please.

> +
> +    if ( set )
> +        memcpy(p + offset, entry, sizeof(struct vdevice_table));
> +    else
> +        memcpy(entry, p + offset, sizeof(struct vdevice_table));

Personally I'd have done this with *entry = *(struct ..*)(p + offset)
and vice versa and let the compiler figure out how to achieve that.

> +    /* dt_entry is validated when read */

The vits_get_vdevice_entry call is right above and in this
implementation I don't see any checks in that function, so I don't think
this comment is strictly true.

Since it is very important that information living in this guest
accessible memory is correctly validated before use I think these
comments need to be 100% trustworthy.

I think vits_get_vdevice_entry is indeed the right place for those
checks as the comment suggests.

If there are no actual checks needed (e.g. because everything is in
terms of IPA) then there should be a comment in that function explaining
exactly why nothing actually needs to be validated.

> +    offset = event * sizeof(struct vitt);
> +    if ( offset > dt_entry.vitt_size )
> +    {
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d ITT out of range\n",
> +                d->domain_id, current->vcpu_id);
> +        return -EINVAL;