[tip: efi/core] efi: cper: fix scnprintf() use in cper_mem_err_location()

[tip: efi/core] efi: cper: fix scnprintf() use in cper_mem_err_location()

[tip-bot2 for Rasmus Villemoes]

The following commit has been merged into the efi/core branch of tip:

Commit-ID:     5eff88dd6b4badd664d7d3b648103d540b390248
Gitweb:        https://git.kernel.org/tip/5eff88dd6b4badd664d7d3b648103d540b390248
Author:        Rasmus Villemoes <linux@rasmusvillemoes.dk>
AuthorDate:    Wed, 21 Apr 2021 21:31:46 +02:00
Committer:     Ard Biesheuvel <ardb@kernel.org>
CommitterDate: Fri, 27 Aug 2021 16:01:27 +02:00

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.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/cper.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index ea7ca74..1cb7097 100644
--- 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)
@@ -258,13 +258,12 @@ static int cper_mem_err_location(struct cper_mem_err_compact *mem, char *msg)
 		n += scnprintf(msg + n, len - n, "responder_id: 0x%016llx ",
 			       mem->responder_id);
 	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
-		scnprintf(msg + n, len - n, "target_id: 0x%016llx ",
-			  mem->target_id);
+		n += scnprintf(msg + n, len - n, "target_id: 0x%016llx ",
+			       mem->target_id);
 	if (mem->validation_bits & CPER_MEM_VALID_CHIP_ID)
-		scnprintf(msg + n, len - n, "chip_id: %d ",
-			  mem->extended >> CPER_MEM_CHIP_ID_SHIFT);
+		n += scnprintf(msg + n, len - n, "chip_id: %d ",
+			       mem->extended >> CPER_MEM_CHIP_ID_SHIFT);
 
-	msg[n] = '\0';
 	return n;
 }
 

Re: [tip: efi/core] efi: cper: fix scnprintf() use in cper_mem_err_location()

[Joe Perches]

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];

Re: [tip: efi/core] efi: cper: fix scnprintf() use in cper_mem_err_location()

[Ard Biesheuvel]

(add RAS/APEI folks)

On Sat, 28 Aug 2021 at 13:31, Joe Perches <joe@perches.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 ...

Re: [tip: efi/core] efi: cper: fix scnprintf() use in cper_mem_err_location()

[James Morse]

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@perches.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

Re: [tip: efi/core] efi: cper: fix scnprintf() use in cper_mem_err_location()

[Ard Biesheuvel]

On Tue, 31 Aug 2021 at 18:02, James Morse <james.morse@arm.com> wrote:
>
> 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@perches.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().
>

OK, better to fix it then - there does not seem to be a good reason
for using a buffer in BSS here anyway.

I'll send out a patch.

Thanks,
Ard.