linux-kernel - Re: [tip: efi/core] efi: cper: fix scnprintf() use in cper_mem_err

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]

Date:   Tue, 31 Aug 2021 17:02:30 +0100
From:   James Morse <james.morse@....com>
To:     Ard Biesheuvel <ardb@...nel.org>, Joe Perches <joe@...ches.com>,
        Borislav Petkov <bp@...en8.de>,
        Tony Luck <tony.luck@...el.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-tip-commits@...r.kernel.org,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        X86 ML <x86@...nel.org>
Subject: Re: [tip: efi/core] efi: cper: fix scnprintf() use in
 cper_mem_err_location()

Hi guys,

On 28/08/2021 13:18, Ard Biesheuvel wrote:
> (add RAS/APEI folks)
> 
> On Sat, 28 Aug 2021 at 13:31, Joe Perches <joe@...ches.com> wrote:
>>
>> On Sat, 2021-08-28 at 10:37 +0000, tip-bot2 for Rasmus Villemoes wrote:
>>> The following commit has been merged into the efi/core branch of tip:
>> []
>>> efi: cper: fix scnprintf() use in cper_mem_err_location()
>>>
>>> The last two if-clauses fail to update n, so whatever they might have
>>> written at &msg[n] would be cut off by the final nul-termination.
>>>
>>> That nul-termination is redundant; scnprintf(), just like snprintf(),
>>> guarantees a nul-terminated output buffer, provided the buffer size is
>>> positive.
>>>
>>> And there's no need to discount one byte from the initial buffer;
>>> vsnprintf() expects to be given the full buffer size - it's not going
>>> to write the nul-terminator one beyond the given (buffer, size) pair.
>> []
>>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> []
>>> @@ -221,7 +221,7 @@ static int cper_mem_err_location(struct cper_mem_err_compact *mem, char *msg)
>>>               return 0;
>>>
>>>
>>>       n = 0;
>>> -     len = CPER_REC_LEN - 1;
>>> +     len = CPER_REC_LEN;
>>>       if (mem->validation_bits & CPER_MEM_VALID_NODE)
>>>               n += scnprintf(msg + n, len - n, "node: %d ", mem->node);
>>>       if (mem->validation_bits & CPER_MEM_VALID_CARD)
>>
>> [etc...]
>>
>> Is this always single threaded?
>>
>> It doesn't seem this is safe for reentry as the output buffer
>> being written into is a single static
>>
>> static char rcd_decode_str[CPER_REC_LEN];

> Good question. CPER error record decoding typically occurs in response
> to an error event raised by firmware, so I think this happens to work
> fine in practice. Whether this is guaranteed, I'm not so sure ...

There is locking to prevent concurrent access to the firmware buffer, but that only
serialises the CPER records being copied. The printing may happen in parallel on different
CPUs if there are multiple errors.

cper_estatus_print() is called in NMI context if an NMI indicates a fatal error. See
__ghes_panic().


Thanks,

James