Re: [PATCH 03/16] x86/traps: Factor out exception_fixup() and make printing consistent

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

On 12/05/2020 14:05, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>
> On 11.05.2020 17:14, Andrew Cooper wrote:
>> On 04/05/2020 14:20, Jan Beulich wrote:
>>> On 02.05.2020 00:58, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/traps.c
>>>> +++ b/xen/arch/x86/traps.c
>>>> @@ -774,10 +774,27 @@ static void do_reserved_trap(struct cpu_user_regs *regs)
>>>>            trapnr, vec_name(trapnr), regs->error_code);
>>>>  }
>>>>  
>>>> +static bool exception_fixup(struct cpu_user_regs *regs, bool print)
>>>> +{
>>>> +    unsigned long fixup = search_exception_table(regs);
>>>> +
>>>> +    if ( unlikely(fixup == 0) )
>>>> +        return false;
>>>> +
>>>> +    /* Can currently be triggered by guests.  Make sure we ratelimit. */
>>>> +    if ( IS_ENABLED(CONFIG_DEBUG) && print )
>>> I didn't think we consider dprintk()-s a possible security issue.
>>> Why would we consider so a printk() hidden behind
>>> IS_ENABLED(CONFIG_DEBUG)? IOW I think one of XENLOG_GUEST and
>>> IS_ENABLED(CONFIG_DEBUG) wants dropping.
>> Who said anything about a security issue?
> The need to rate limit is (among other aspects) to prevent a
> (logspam) security issue, isn't it?

Rate limiting (from a security aspect) is a stopgap solution to relieve
incidental pressure on the various global spinlocks involved.

It specifically does not prevent a guest from trivially filling the
console ring with junk, or for that junk to be written to
/var/log/xen/hypervisor.log at an alarming rate, both of which are
issues in production setups, but not security issues.

Technical solutions to these problems do exist, such as deleting the
offending printk(), or maintaining per-guest console rings, but both
come with downsides in terms of usability, which similarly impacts
production setups.


What ratelimiting even in debug builds gets you is a quick spate of
printks() (e.g. any new sshd connection on an AMD system where the
MSR_VIRT_SPEC_CTRL patch is still uncommitted, and the default WRMSR
behaviour breaking wrmsr_safe() logic in Linux) not wasting an
unreasonable quantity of space in the console ring.

~Andrew