diff mbox series

[v2,06/14] powerpc/pseries/ras: FWNMI_VALID off by one

Message ID 20200403132622.130394-7-npiggin@gmail.com (mailing list archive)
State Superseded
Headers show
Series powerpc/64: machine check and system reset fixes | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (d0c12846a3a24cd6d68b608c866712bc7e471634)
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 13 lines checked
snowpatch_ozlabs/needsstable success Patch has no Fixes tags

Commit Message

Nicholas Piggin April 3, 2020, 1:26 p.m. UTC
This was discovered developing qemu fwnmi sreset support. This
off-by-one bug means the last 16 bytes of the rtas area can not
be used for a 16 byte save area.

It's not a serious bug, and QEMU implementation has to retain a
workaround for old kernels, but it's good to tighten it.

Acked-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/pseries/ras.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Christophe Leroy April 3, 2020, 2:33 p.m. UTC | #1
Le 03/04/2020 à 15:26, Nicholas Piggin a écrit :
> This was discovered developing qemu fwnmi sreset support. This
> off-by-one bug means the last 16 bytes of the rtas area can not
> be used for a 16 byte save area.
> 
> It's not a serious bug, and QEMU implementation has to retain a
> workaround for old kernels, but it's good to tighten it.
> 
> Acked-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/platforms/pseries/ras.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
> index c74d5e740922..9a37bda47468 100644
> --- a/arch/powerpc/platforms/pseries/ras.c
> +++ b/arch/powerpc/platforms/pseries/ras.c
> @@ -395,10 +395,11 @@ static irqreturn_t ras_error_interrupt(int irq, void *dev_id)
>   /*
>    * Some versions of FWNMI place the buffer inside the 4kB page starting at
>    * 0x7000. Other versions place it inside the rtas buffer. We check both.
> + * Minimum size of the buffer is 16 bytes.
>    */
>   #define VALID_FWNMI_BUFFER(A) \
> -	((((A) >= 0x7000) && ((A) < 0x7ff0)) || \
> -	(((A) >= rtas.base) && ((A) < (rtas.base + rtas.size - 16))))
> +	((((A) >= 0x7000) && ((A) <= 0x8000 - 16)) || \
> +	(((A) >= rtas.base) && ((A) <= (rtas.base + rtas.size - 16))))

Why not just change < to <= and leave the 0x7ff0 ?

>   
>   static inline struct rtas_error_log *fwnmi_get_errlog(void)
>   {
> 

Christophe
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index c74d5e740922..9a37bda47468 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -395,10 +395,11 @@  static irqreturn_t ras_error_interrupt(int irq, void *dev_id)
 /*
  * Some versions of FWNMI place the buffer inside the 4kB page starting at
  * 0x7000. Other versions place it inside the rtas buffer. We check both.
+ * Minimum size of the buffer is 16 bytes.
  */
 #define VALID_FWNMI_BUFFER(A) \
-	((((A) >= 0x7000) && ((A) < 0x7ff0)) || \
-	(((A) >= rtas.base) && ((A) < (rtas.base + rtas.size - 16))))
+	((((A) >= 0x7000) && ((A) <= 0x8000 - 16)) || \
+	(((A) >= rtas.base) && ((A) <= (rtas.base + rtas.size - 16))))
 
 static inline struct rtas_error_log *fwnmi_get_errlog(void)
 {