Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Nathan Lynch <nathanl@linux.ibm.com>
To:     Libor Pechacek <lpechacek@suse.cz>
Cc:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Michal Suchanek <msuchanek@suse.cz>,
        Thomas Gleixner <tglx@linutronix.de>,
        Allison Randal <allison@lohutok.net>,
        Leonardo Bras <leonardo@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] powerpc: drmem: avoid NULL pointer dereference when drmem is unavailable
In-Reply-To: <20200116102758.GC25138@fm.suse.cz>
References: <20200116102758.GC25138@fm.suse.cz>
Date:   Thu, 23 Jan 2020 09:56:10 -0600
Message-ID: <87o8uudv51.fsf@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello and thanks for the patch.

Libor Pechacek <lpechacek@suse.cz> writes:
> In KVM guests drmem structure is only zero initialized. Trying to
> manipulate DLPAR parameters results in a crash in this environment.

I think this statement needs qualification. Unless I'm mistaken, this
happens only when you boot a guest without any hotpluggable memory
configured, and then try to add or remove memory.


> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index 3d76e1c388c2..28c3d936fdf3 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -27,12 +27,12 @@ struct drmem_lmb_info {
>  extern struct drmem_lmb_info *drmem_info;
>  
>  #define for_each_drmem_lmb_in_range(lmb, start, end)		\
> -	for ((lmb) = (start); (lmb) <= (end); (lmb)++)
> +	for ((lmb) = (start); (lmb) < (end); (lmb)++)
>  
>  #define for_each_drmem_lmb(lmb)					\
>  	for_each_drmem_lmb_in_range((lmb),			\
>  		&drmem_info->lmbs[0],				\
> -		&drmem_info->lmbs[drmem_info->n_lmbs - 1])
> +		&drmem_info->lmbs[drmem_info->n_lmbs])
>  
>  /*
>   * The of_drconf_cell_v1 struct defines the layout of the LMB data
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index c126b94d1943..4ea6af002e27 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -236,9 +236,9 @@ static int get_lmb_range(u32 drc_index, int n_lmbs,
>  	if (!start)
>  		return -EINVAL;
>  
> -	end = &start[n_lmbs - 1];
> +	end = &start[n_lmbs];
>  
> -	last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs - 1];
> +	last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs];
>  	if (end > last_lmb)
>  		return -EINVAL;

Is this not undefined behavior? I'd rather do this in a way that does
not involve forming out-of-bounds pointers. Even if it's safe, naming
that pointer "last_lmb" now actively hinders understanding of the code;
it should be named "limit" or something.

Anyway, I confess I've had the following patch sitting unsubmitted due
to a combination of perceived low urgency and lack of time. I've
verified it addresses your reported problem but I need a day or two to
check that it doesn't regress memory add/remove on suitably configured
guests:

================================================================

powerpc/drmem: make drmem list traversal safe on non-drmem systems

A user has reported a crash when attempting to remove an LMB on a KVM
guest where the device tree lacks the nodes and properties
(ibm,dynamic-reconfiguration-memory etc) which allow the drmem code to
support dynamic memory removal:

pseries-hotplug-mem: Attempting to hot-remove 1 LMB(s)
Unable to handle kernel paging request for data at address 0x00000010
Faulting instruction address: 0xc0000000000f0a30
Oops: Kernel access of bad area, sig: 11 [#1]
[... regs etc ...]
NIP [c0000000000f0a30] dlpar_memory+0x510/0xb50
LR [c0000000000f09e8] dlpar_memory+0x4c8/0xb50
Call Trace:
[c0000001edf97ba0] [c0000000000f09e8] dlpar_memory+0x4c8/0xb50 (unreliable)
[c0000001edf97c20] [c0000000000e8390] handle_dlpar_errorlog+0x60/0x140
[c0000001edf97c90] [c0000000000e85e0] dlpar_store+0x170/0x490
[c0000001edf97d40] [c000000000cb0c90] kobj_attr_store+0x30/0x50
[c0000001edf97d60] [c000000000591598] sysfs_kf_write+0x68/0xa0
[c0000001edf97d80] [c00000000058fec4] kernfs_fop_write+0x104/0x270
[c0000001edf97dd0] [c0000000004a1078] sys_write+0x128/0x380
[c0000001edf97e30] [c00000000000b388] system_call+0x5c/0x70

Make for_each_drmem_lmb() safe for the case where the list of drmem
LMBs is unpopulated.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

1 file changed, 36 insertions(+), 7 deletions(-)
arch/powerpc/include/asm/drmem.h | 43 +++++++++++++++++++++++++++++++++-------

modified   arch/powerpc/include/asm/drmem.h
@@ -20,19 +20,48 @@ struct drmem_lmb {
 
 struct drmem_lmb_info {
 	struct drmem_lmb        *lmbs;
-	int                     n_lmbs;
+	unsigned int            n_lmbs;
 	u32                     lmb_size;
 };
 
 extern struct drmem_lmb_info *drmem_info;
 
-#define for_each_drmem_lmb_in_range(lmb, start, end)		\
-	for ((lmb) = (start); (lmb) <= (end); (lmb)++)
+static inline bool drmem_present(void)
+{
+	return drmem_info->lmbs != NULL;
+}
+
+static inline struct drmem_lmb *drmem_lmb_index(unsigned int index)
+{
+	if (!drmem_present())
+		return NULL;
 
-#define for_each_drmem_lmb(lmb)					\
-	for_each_drmem_lmb_in_range((lmb),			\
-		&drmem_info->lmbs[0],				\
-		&drmem_info->lmbs[drmem_info->n_lmbs - 1])
+	if (WARN_ON(index >= drmem_info->n_lmbs))
+		return NULL;
+
+	return &drmem_info->lmbs[index];
+}
+
+static inline struct drmem_lmb *drmem_first_lmb(void)
+{
+	return drmem_lmb_index(0);
+}
+
+static inline struct drmem_lmb *drmem_last_lmb(void)
+{
+	if (!drmem_present())
+		return NULL;
+
+	return drmem_lmb_index(drmem_info->n_lmbs - 1);
+}
+
+#define for_each_drmem_lmb(lmb)						\
+	for ((lmb) = drmem_first_lmb();					\
+	     (lmb) != NULL && (lmb) <= drmem_last_lmb();		\
+	     (lmb)++)
+
+#define for_each_drmem_lmb_in_range(lmb, start, end)	\
+	for ((lmb) = (start); (lmb) <= (end); (lmb)++)
 
 /*
  * The of_drconf_cell_v1 struct defines the layout of the LMB data