linux-kernel - Re: [PATCH v2 07/11] arm64: mops: handle MOPS exceptions

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 30 May 2023 17:36:18 +0100
From:   Kristina Martsenko <kristina.martsenko@....com>
To:     Colton Lewis <coltonlewis@...gle.com>
Cc:     linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Oliver Upton <oliver.upton@...ux.dev>,
        James Morse <james.morse@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Zenghui Yu <yuzenghui@...wei.com>,
        Mark Rutland <mark.rutland@....com>,
        Mark Brown <broonie@...nel.org>,
        Luis Machado <luis.machado@....com>,
        Vladimir Murzin <vladimir.murzin@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 07/11] arm64: mops: handle MOPS exceptions

On 25/05/2023 20:50, Colton Lewis wrote:
>> +    if (esr & ESR_ELx_MOPS_ISS_MEM_INST) {
>> +        /* SET* instruction */
>> +        if (option_a ^ wrong_option) {
>> +            /* Format is from Option A; forward set */
>> +            pt_regs_write_reg(regs, dstreg, dst + size);
>> +            pt_regs_write_reg(regs, sizereg, -size);
>> +        }
>> +    } else {
>> +        /* CPY* instruction */
>> +        if (!(option_a ^ wrong_option)) {
>> +            /* Format is from Option B */
>> +            if (regs->pstate & PSR_N_BIT) {
>> +                /* Backward copy */
>> +                pt_regs_write_reg(regs, dstreg, dst - size);
>> +                pt_regs_write_reg(regs, srcreg, src - size);
>> +            }
>> +        } else {
>> +            /* Format is from Option A */
>> +            if (size & BIT(63)) {
>> +                /* Forward copy */
>> +                pt_regs_write_reg(regs, dstreg, dst + size);
>> +                pt_regs_write_reg(regs, srcreg, src + size);
>> +                pt_regs_write_reg(regs, sizereg, -size);
>> +            }
>> +        }
>> +    }
> 
> I can see an argument for styling things closely to the ARM manual as
> you have done here, but Linux style recommends against deep nesting. In
> this case it is unneeded. I believe this can be written as a single
> if-else chain and that makes it easier to distinguish the three options.
> 
> if ((esr & ESR_ELx_MOPS_ISS_MEM_INST) && (option_a ^ wrong_option)) {
>     /* Format is from Option A; forward set */
>     pt_regs_write_reg(regs, dstreg, dst + size);
>     pt_regs_write_reg(regs, sizereg, -size);
> } else if ((option_a ^ wrong_option) && (size & BIT(63)) {
>     /* Forward copy */
>     pt_regs_write_reg(regs, dstreg, dst + size);
>     pt_regs_write_reg(regs, srcreg, src + size);
>     pt_regs_write_reg(regs, sizereg, -size);
> } else if (regs-pstate & PSR_N_BIT) {
>     /* Backward copy */
>     pt_regs_write_reg(regs, dstreg, dst - size);
>     pt_regs_write_reg(regs, srcreg, src - size);
> }

Yeah, the nesting gets a bit deep here, but there are 6 cases in total, ie 6 
ways the hardware can set up the registers and pstate (in 3 of them the kernel
doesn't need to modify the registers), and I think the current structure makes
it clearer what the 6 are, so I'd prefer to keep it as it is for now.

Thanks,
Kristina