linux-kernel - Re: [PATCH] x86/iopl: Fake iopl(3) CLI/STI usage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wj_-bRjcJdXubdUUsTph+DH-5f77FmkbNCfFVYg=Td2zw@mail.gmail.com>
Date:   Fri, 17 Sep 2021 15:23:18 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ondrej Zary <linux@...y.sk>, Thomas Gleixner <tglx@...utronix.de>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/iopl: Fake iopl(3) CLI/STI usage

On Fri, Sep 17, 2021 at 2:21 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> +       nr_copied = insn_fetch_from_user(regs, buf);

Ugh. This is the code that does the magic "add CS base" stuff.

Do we really want to do that instead of just doing

        unsigned char byte = get_user((char __user *)regs->ip);

when later on the debug code does:

> +               pr_err("%s[%d] attempts to use CLI/STI, pretending it's a NOP, ip:%lx",
> +                      current->comm, task_pid_nr(current), regs->ip);
> +               print_vma_addr(KERN_CONT " in ", regs->ip);
> +               pr_cont("\n");

and prints out the wrong IP address?

IOW, I'd argue that you should get it right in both places, or not try
to get it right in one but not the other.

I think the proper thing to do is perhaps something like

        unsigned long cs_base = 0;
        unsigned long address;
        unsigned char byte;

        if (!user_64bit_mode(regs)) {
                cs_base = insn_get_seg_base(regs, INAT_SEG_REG_CS);
                if (cs_base = -1ul)
                        return false;
        }

        // We could check the limit too, but nobody cares
        address = regs->ip + cs_base;
        if (get_user(byte, (const char __user *)address))
                return false;

        // cli/sti?
        if (byte != 0xfa && byte ! 0xfb)
                return false;

and now you have the actual linear address in 'address' and can at
least print it out correctly.

Hmm? Because it's just sad to get it right in one place, and wrong in
another. And we don't actually _want_ any of the instruction
fetch/decode stuff.

           Linus