Re: [Xen-devel] BUG_ON() vs ASSERT()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/09/16 16:23, Jan Beulich wrote:
> All,
>
> in
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01201.html
> and
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01210.html
> Andrew basically suggests that we should switch away from using
> ASSERT() and over to BUG_ON() in perhaps quite broad a set of
> cases. And honestly I'm not convinced of this: We've been adding
> quite a few ASSERT()s over the last years with the aim of doing
> sanity checking in debug builds, without adding overhead to non-
> debug builds. I can certainly see possible cases where using
> BUG_ON() to prevent further possible damage is appropriate, but
> I don't think we should overdo here.

I am not advocating switching all ASSERT()s to BUG_ON()s.  That would be
silly.

However, ASSERT()'s as a bounds check very definitely are dangerous.  If
there is any uncertainty about the bounds, the check must not disappear
in a release build.  (Better yet, code which copes cleanly with
insufficient bounds).


For anyone reading this email who hasn't already worked out the details
of XSA-186, the data corruption issue is here:

static int hvmemul_insn_fetch(...)
{
    unsigned int insn_off = offset - hvmemul_ctxt->insn_buf_eip;
    ...
    ASSERT(insn_off + bytes <= sizeof(hvmemul_ctxt->insn_buf));
    memcpy(&hvmemul_ctxt->insn_buf[insn_off], p_data, bytes);
    ...

It is left as an exercise to the reader to work out how to exploit this
on a release build of Xen, but it is hopefully obvious that the ASSERT()
isn't helpful.  A BUG_ON() in this case would have been a host DoS,
which is substantially better than a guest escape.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel