In KVM guests drmem structure is only zero initialized. Trying to
manipulate DLPAR parameters results in a crash in this environment.
$ echo "memory add count 1" > /sys/kernel/dlpar
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: af_packet(E) rfkill(E) nvram(E) vmx_crypto(E)
gf128mul(E) e1000(E) virtio_balloon(E) rtc_generic(E) crct10dif_vpmsum(E)
btrfs(E) blake2b_generic(E) libcrc32c(E) xor(E) raid6_pq(E) virtio_rng(E)
virtio_blk(E) ohci_pci(E) ehci_pci(E) ohci_hcd(E) ehci_hcd(E)
crc32c_vpmsum(E) usbcore(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E)
dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E)
scsi_mod(E)
CPU: 1 PID: 4114 Comm: bash Kdump: loaded Tainted: G E 5.5.0-rc6-2-default #1
NIP: c0000000000ff294 LR: c0000000000ff248 CTR: 0000000000000000
REGS: c0000000fb9d3880 TRAP: 0300 Tainted: G E (5.5.0-rc6-2-default)
MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 28242428 XER: 20000000
CFAR: c0000000009a6c10 DAR: 0000000000000010 DSISR: 40000000 IRQMASK: 0
GPR00: c0000000000ff248 c0000000fb9d3b10 c000000001682e00 0000000000000033
GPR04: c0000000ff30bf90 c0000000ff394800 0000000000005110 ffffffffffffffe8
GPR08: 0000000000000000 0000000000000000 00000000fe1c0000 0000000000000000
GPR12: 0000000000002200 c00000003fffee00 0000000000000000 000000011cbc37c0
GPR16: 000000011cb27ed0 0000000000000000 000000011cb6dd10 0000000000000000
GPR20: 000000011cb7db28 000001003ce035f0 000000011cbc7828 000000011cbc6c70
GPR24: 000001003cf01210 0000000000000000 c0000000ffade4e0 c000000002d7216b
GPR28: 0000000000000001 c000000002d78560 0000000000000000 c0000000015458d0
NIP [c0000000000ff294] dlpar_memory+0x6e4/0xd00
LR [c0000000000ff248] dlpar_memory+0x698/0xd00
Call Trace:
[c0000000fb9d3b10] [c0000000000ff248] dlpar_memory+0x698/0xd00 (unreliable)
[c0000000fb9d3ba0] [c0000000000f5990] handle_dlpar_errorlog+0xc0/0x190
[c0000000fb9d3c10] [c0000000000f5c58] dlpar_store+0x198/0x4a0
[c0000000fb9d3cd0] [c000000000c4cb00] kobj_attr_store+0x30/0x50
[c0000000fb9d3cf0] [c0000000005a37b4] sysfs_kf_write+0x64/0x90
[c0000000fb9d3d10] [c0000000005a2c90] kernfs_fop_write+0x1b0/0x290
[c0000000fb9d3d60] [c0000000004a2bec] __vfs_write+0x3c/0x70
[c0000000fb9d3d80] [c0000000004a6560] vfs_write+0xd0/0x260
[c0000000fb9d3dd0] [c0000000004a69ac] ksys_write+0xdc/0x130
[c0000000fb9d3e20] [c00000000000b478] system_call+0x5c/0x68
Instruction dump:
ebc90000 1ce70018 38e7ffe8 7cfe3a14 7fbe3840 419dff14 fb610068 7fc9f378
39000000 4800000c 60000000 4195fef4 <81490010> 39290018 38c80001 7ea93840
---[ end trace cc2dd8152608c295 ]---
Taking closer look at the code, I can see that for_each_drmem_lmb is a
macro expanding into `for (lmb = &drmem_info->lmbs[0]; lmb <=
&drmem_info->lmbs[drmem_info->n_lmbs - 1]; lmb++)`. When drmem_info->lmbs
is NULL, the loop would iterate through the whole address range if it
weren't stopped by the NULL pointer dereference on the next line.
This patch aligns for_each_drmem_lmb and for_each_drmem_lmb_in_range macro
behavior with the common C semantics, where the end marker does not belong
to the scanned range, and alters get_lmb_range() semantics. As a side
effect, the wraparound observed in the crash is prevented.
Fixes: 6c6ea53725b3 ("powerpc/mm: Separate ibm, dynamic-memory data from DT format")
Cc: Michal Suchanek <[email protected]>
Cc: [email protected]
Signed-off-by: Libor Pechacek <[email protected]>
---
arch/powerpc/include/asm/drmem.h | 4 ++--
arch/powerpc/platforms/pseries/hotplug-memory.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
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;
--
2.24.1
--
Libor Pechacek
SUSE Labs Remember to have fun...
On Thu, Jan 16, 2020 at 11:27:58AM +0100, Libor Pechacek wrote:
> In KVM guests drmem structure is only zero initialized. Trying to
> manipulate DLPAR parameters results in a crash in this environment.
>
> $ echo "memory add count 1" > /sys/kernel/dlpar
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in: af_packet(E) rfkill(E) nvram(E) vmx_crypto(E)
> gf128mul(E) e1000(E) virtio_balloon(E) rtc_generic(E) crct10dif_vpmsum(E)
> btrfs(E) blake2b_generic(E) libcrc32c(E) xor(E) raid6_pq(E) virtio_rng(E)
> virtio_blk(E) ohci_pci(E) ehci_pci(E) ohci_hcd(E) ehci_hcd(E)
> crc32c_vpmsum(E) usbcore(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E)
> dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E)
> scsi_mod(E)
> CPU: 1 PID: 4114 Comm: bash Kdump: loaded Tainted: G E 5.5.0-rc6-2-default #1
> NIP: c0000000000ff294 LR: c0000000000ff248 CTR: 0000000000000000
> REGS: c0000000fb9d3880 TRAP: 0300 Tainted: G E (5.5.0-rc6-2-default)
> MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 28242428 XER: 20000000
> CFAR: c0000000009a6c10 DAR: 0000000000000010 DSISR: 40000000 IRQMASK: 0
> GPR00: c0000000000ff248 c0000000fb9d3b10 c000000001682e00 0000000000000033
> GPR04: c0000000ff30bf90 c0000000ff394800 0000000000005110 ffffffffffffffe8
> GPR08: 0000000000000000 0000000000000000 00000000fe1c0000 0000000000000000
> GPR12: 0000000000002200 c00000003fffee00 0000000000000000 000000011cbc37c0
> GPR16: 000000011cb27ed0 0000000000000000 000000011cb6dd10 0000000000000000
> GPR20: 000000011cb7db28 000001003ce035f0 000000011cbc7828 000000011cbc6c70
> GPR24: 000001003cf01210 0000000000000000 c0000000ffade4e0 c000000002d7216b
> GPR28: 0000000000000001 c000000002d78560 0000000000000000 c0000000015458d0
> NIP [c0000000000ff294] dlpar_memory+0x6e4/0xd00
> LR [c0000000000ff248] dlpar_memory+0x698/0xd00
> Call Trace:
> [c0000000fb9d3b10] [c0000000000ff248] dlpar_memory+0x698/0xd00 (unreliable)
> [c0000000fb9d3ba0] [c0000000000f5990] handle_dlpar_errorlog+0xc0/0x190
> [c0000000fb9d3c10] [c0000000000f5c58] dlpar_store+0x198/0x4a0
> [c0000000fb9d3cd0] [c000000000c4cb00] kobj_attr_store+0x30/0x50
> [c0000000fb9d3cf0] [c0000000005a37b4] sysfs_kf_write+0x64/0x90
> [c0000000fb9d3d10] [c0000000005a2c90] kernfs_fop_write+0x1b0/0x290
> [c0000000fb9d3d60] [c0000000004a2bec] __vfs_write+0x3c/0x70
> [c0000000fb9d3d80] [c0000000004a6560] vfs_write+0xd0/0x260
> [c0000000fb9d3dd0] [c0000000004a69ac] ksys_write+0xdc/0x130
> [c0000000fb9d3e20] [c00000000000b478] system_call+0x5c/0x68
> Instruction dump:
> ebc90000 1ce70018 38e7ffe8 7cfe3a14 7fbe3840 419dff14 fb610068 7fc9f378
> 39000000 4800000c 60000000 4195fef4 <81490010> 39290018 38c80001 7ea93840
> ---[ end trace cc2dd8152608c295 ]---
>
> Taking closer look at the code, I can see that for_each_drmem_lmb is a
> macro expanding into `for (lmb = &drmem_info->lmbs[0]; lmb <=
> &drmem_info->lmbs[drmem_info->n_lmbs - 1]; lmb++)`. When drmem_info->lmbs
> is NULL, the loop would iterate through the whole address range if it
> weren't stopped by the NULL pointer dereference on the next line.
>
> This patch aligns for_each_drmem_lmb and for_each_drmem_lmb_in_range macro
> behavior with the common C semantics, where the end marker does not belong
> to the scanned range, and alters get_lmb_range() semantics. As a side
> effect, the wraparound observed in the crash is prevented.
>
> Fixes: 6c6ea53725b3 ("powerpc/mm: Separate ibm, dynamic-memory data from DT format")
> Cc: Michal Suchanek <[email protected]>
> Cc: [email protected]
> Signed-off-by: Libor Pechacek <[email protected]>
Reviewed-by: Michal Suchanek <[email protected]>
> ---
> arch/powerpc/include/asm/drmem.h | 4 ++--
> arch/powerpc/platforms/pseries/hotplug-memory.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> 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;
>
> --
> 2.24.1
>
>
> --
> Libor Pechacek
> SUSE Labs Remember to have fun...
Hello and thanks for the patch.
Libor Pechacek <[email protected]> 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 <[email protected]>
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
On Thu, Jan 23, 2020 at 09:56:10AM -0600, Nathan Lynch wrote:
> Hello and thanks for the patch.
>
> Libor Pechacek <[email protected]> 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.
Indeed, the name might be misleading now.
However, the loop differes from anything else we have in the kernel.
The standard explicitly allows the pointer to point just after the last
element to allow expressing the iteration limit without danger of
overflow.
Thanks
Michal
Hello Nathan,
On Thu 23-01-20 09:56:10, Nathan Lynch wrote:
> Libor Pechacek <[email protected]> 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.
Thanks for the review. The introductory statement can indeed be clearer.
[...]
> > 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.
Well, this is a tough question for the case when drmem_info->lmbs was not
allocated. Given that the array does not exist, what bounds are we talking
about?
My patch builds on the fact that NULL[0] is NULL and NULL < NULL is false.
Talking about a pointer to one past the last element of an non-existent array
is too much philosophy for me.
For the case when drmem_info->lmbs is allocated, last_lmb is a pointer to one
past the last element of the array as Michal mentioned.
> Even if it's safe, naming that pointer "last_lmb" now actively hinders
> understanding of the code; it should be named "limit" or something.
Good catch.
[...]
> 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;
> +}
Yes, use of this test was also my first idea about the fix.
> +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;
Why is this WARN_ON needed?
> +
> + 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);
Is the unsigned integer wraparound intended in drmem_info->n_lmbs == 0 case?
> +}
> +
> +#define for_each_drmem_lmb(lmb) \
> + for ((lmb) = drmem_first_lmb(); \
drmem_first_lmb() is essentially a call to drmem_info->lmbs(0). What
happens if drmem_info->n_lmbs is zero and drmem_info->lmbs is not NULL?
> + (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
>
After all, I don't mind how the bug will be fixed. As you can see, my
preference is towards simpler solutions.
In my opinion your solution special-cased drmem_info->lmbs == NULL and opened
the doorway to the combination of drmem_info->lmbs != NULL &&
!drmem_info->n_lmbs. Maybe the condition can never become true but the code
should IMHO be robust enough to handle it.
Thanks!
Libor
--
Libor Pechacek
SUSE Labs Remember to have fun...
From: Libor Pechacek <[email protected]>
In guests without hotplugagble memory drmem structure is only zero
initialized. Trying to manipulate DLPAR parameters results in a crash.
$ echo "memory add count 1" > /sys/kernel/dlpar
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: af_packet(E) rfkill(E) nvram(E) vmx_crypto(E)
gf128mul(E) e1000(E) virtio_balloon(E) rtc_generic(E) crct10dif_vpmsum(E)
btrfs(E) blake2b_generic(E) libcrc32c(E) xor(E) raid6_pq(E) virtio_rng(E)
virtio_blk(E) ohci_pci(E) ehci_pci(E) ohci_hcd(E) ehci_hcd(E)
crc32c_vpmsum(E) usbcore(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E)
dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E)
scsi_mod(E)
CPU: 1 PID: 4114 Comm: bash Kdump: loaded Tainted: G E 5.5.0-rc6-2-default #1
NIP: c0000000000ff294 LR: c0000000000ff248 CTR: 0000000000000000
REGS: c0000000fb9d3880 TRAP: 0300 Tainted: G E (5.5.0-rc6-2-default)
MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 28242428 XER: 20000000
CFAR: c0000000009a6c10 DAR: 0000000000000010 DSISR: 40000000 IRQMASK: 0
GPR00: c0000000000ff248 c0000000fb9d3b10 c000000001682e00 0000000000000033
GPR04: c0000000ff30bf90 c0000000ff394800 0000000000005110 ffffffffffffffe8
GPR08: 0000000000000000 0000000000000000 00000000fe1c0000 0000000000000000
GPR12: 0000000000002200 c00000003fffee00 0000000000000000 000000011cbc37c0
GPR16: 000000011cb27ed0 0000000000000000 000000011cb6dd10 0000000000000000
GPR20: 000000011cb7db28 000001003ce035f0 000000011cbc7828 000000011cbc6c70
GPR24: 000001003cf01210 0000000000000000 c0000000ffade4e0 c000000002d7216b
GPR28: 0000000000000001 c000000002d78560 0000000000000000 c0000000015458d0
NIP [c0000000000ff294] dlpar_memory+0x6e4/0xd00
LR [c0000000000ff248] dlpar_memory+0x698/0xd00
Call Trace:
[c0000000fb9d3b10] [c0000000000ff248] dlpar_memory+0x698/0xd00 (unreliable)
[c0000000fb9d3ba0] [c0000000000f5990] handle_dlpar_errorlog+0xc0/0x190
[c0000000fb9d3c10] [c0000000000f5c58] dlpar_store+0x198/0x4a0
[c0000000fb9d3cd0] [c000000000c4cb00] kobj_attr_store+0x30/0x50
[c0000000fb9d3cf0] [c0000000005a37b4] sysfs_kf_write+0x64/0x90
[c0000000fb9d3d10] [c0000000005a2c90] kernfs_fop_write+0x1b0/0x290
[c0000000fb9d3d60] [c0000000004a2bec] __vfs_write+0x3c/0x70
[c0000000fb9d3d80] [c0000000004a6560] vfs_write+0xd0/0x260
[c0000000fb9d3dd0] [c0000000004a69ac] ksys_write+0xdc/0x130
[c0000000fb9d3e20] [c00000000000b478] system_call+0x5c/0x68
Instruction dump:
ebc90000 1ce70018 38e7ffe8 7cfe3a14 7fbe3840 419dff14 fb610068 7fc9f378
39000000 4800000c 60000000 4195fef4 <81490010> 39290018 38c80001 7ea93840
---[ end trace cc2dd8152608c295 ]---
Taking closer look at the code, I can see that for_each_drmem_lmb is a
macro expanding into `for (lmb = &drmem_info->lmbs[0]; lmb <=
&drmem_info->lmbs[drmem_info->n_lmbs - 1]; lmb++)`. When drmem_info->lmbs
is NULL, the loop would iterate through the whole address range if it
weren't stopped by the NULL pointer dereference on the next line.
This patch aligns for_each_drmem_lmb and for_each_drmem_lmb_in_range macro
behavior with the common C semantics, where the end marker does not belong
to the scanned range, and alters get_lmb_range() semantics. As a side
effect, the wraparound observed in the crash is prevented.
Fixes: 6c6ea53725b3 ("powerpc/mm: Separate ibm, dynamic-memory data from DT format")
Cc: Michal Suchanek <[email protected]>
Cc: [email protected]
Signed-off-by: Libor Pechacek <[email protected]>
Signed-off-by: Michal Suchanek <[email protected]>
---
v2: rename last_lmb -> limit, clarify error condition.
---
arch/powerpc/include/asm/drmem.h | 4 ++--
arch/powerpc/platforms/pseries/hotplug-memory.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
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..a6b207dd1d7d 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -223,7 +223,7 @@ static int get_lmb_range(u32 drc_index, int n_lmbs,
struct drmem_lmb **end_lmb)
{
struct drmem_lmb *lmb, *start, *end;
- struct drmem_lmb *last_lmb;
+ struct drmem_lmb *limit;
start = NULL;
for_each_drmem_lmb(lmb) {
@@ -236,10 +236,10 @@ 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];
- if (end > last_lmb)
+ limit = &drmem_info->lmbs[drmem_info->n_lmbs];
+ if (end > limit)
return -EINVAL;
*start_lmb = start;
--
2.23.0
Michal Suchanek <[email protected]> writes:
> From: Libor Pechacek <[email protected]>
>
> In guests without hotplugagble memory drmem structure is only zero
> initialized. Trying to manipulate DLPAR parameters results in a crash.
>
[...]
>
> Fixes: 6c6ea53725b3 ("powerpc/mm: Separate ibm, dynamic-memory data from DT format")
> Cc: Michal Suchanek <[email protected]>
> Cc: [email protected]
> Signed-off-by: Libor Pechacek <[email protected]>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> v2: rename last_lmb -> limit, clarify error condition.
Acked-by: Nathan Lynch <[email protected]>
Thanks!
On Fri, 2020-01-31 at 13:28:29 UTC, Michal Suchanek wrote:
>
> From: Libor Pechacek <[email protected]>
>
> In guests without hotplugagble memory drmem structure is only zero
> initialized. Trying to manipulate DLPAR parameters results in a crash.
>
> $ echo "memory add count 1" > /sys/kernel/dlpar
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in: af_packet(E) rfkill(E) nvram(E) vmx_crypto(E)
> gf128mul(E) e1000(E) virtio_balloon(E) rtc_generic(E) crct10dif_vpmsum(E)
> btrfs(E) blake2b_generic(E) libcrc32c(E) xor(E) raid6_pq(E) virtio_rng(E)
> virtio_blk(E) ohci_pci(E) ehci_pci(E) ohci_hcd(E) ehci_hcd(E)
> crc32c_vpmsum(E) usbcore(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E)
> dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E)
> scsi_mod(E)
> CPU: 1 PID: 4114 Comm: bash Kdump: loaded Tainted: G E 5.5.0-rc6-2-default #1
> NIP: c0000000000ff294 LR: c0000000000ff248 CTR: 0000000000000000
> REGS: c0000000fb9d3880 TRAP: 0300 Tainted: G E (5.5.0-rc6-2-default)
> MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 28242428 XER: 20000000
> CFAR: c0000000009a6c10 DAR: 0000000000000010 DSISR: 40000000 IRQMASK: 0
> GPR00: c0000000000ff248 c0000000fb9d3b10 c000000001682e00 0000000000000033
> GPR04: c0000000ff30bf90 c0000000ff394800 0000000000005110 ffffffffffffffe8
> GPR08: 0000000000000000 0000000000000000 00000000fe1c0000 0000000000000000
> GPR12: 0000000000002200 c00000003fffee00 0000000000000000 000000011cbc37c0
> GPR16: 000000011cb27ed0 0000000000000000 000000011cb6dd10 0000000000000000
> GPR20: 000000011cb7db28 000001003ce035f0 000000011cbc7828 000000011cbc6c70
> GPR24: 000001003cf01210 0000000000000000 c0000000ffade4e0 c000000002d7216b
> GPR28: 0000000000000001 c000000002d78560 0000000000000000 c0000000015458d0
> NIP [c0000000000ff294] dlpar_memory+0x6e4/0xd00
> LR [c0000000000ff248] dlpar_memory+0x698/0xd00
> Call Trace:
> [c0000000fb9d3b10] [c0000000000ff248] dlpar_memory+0x698/0xd00 (unreliable)
> [c0000000fb9d3ba0] [c0000000000f5990] handle_dlpar_errorlog+0xc0/0x190
> [c0000000fb9d3c10] [c0000000000f5c58] dlpar_store+0x198/0x4a0
> [c0000000fb9d3cd0] [c000000000c4cb00] kobj_attr_store+0x30/0x50
> [c0000000fb9d3cf0] [c0000000005a37b4] sysfs_kf_write+0x64/0x90
> [c0000000fb9d3d10] [c0000000005a2c90] kernfs_fop_write+0x1b0/0x290
> [c0000000fb9d3d60] [c0000000004a2bec] __vfs_write+0x3c/0x70
> [c0000000fb9d3d80] [c0000000004a6560] vfs_write+0xd0/0x260
> [c0000000fb9d3dd0] [c0000000004a69ac] ksys_write+0xdc/0x130
> [c0000000fb9d3e20] [c00000000000b478] system_call+0x5c/0x68
> Instruction dump:
> ebc90000 1ce70018 38e7ffe8 7cfe3a14 7fbe3840 419dff14 fb610068 7fc9f378
> 39000000 4800000c 60000000 4195fef4 <81490010> 39290018 38c80001 7ea93840
> ---[ end trace cc2dd8152608c295 ]---
>
> Taking closer look at the code, I can see that for_each_drmem_lmb is a
> macro expanding into `for (lmb = &drmem_info->lmbs[0]; lmb <=
> &drmem_info->lmbs[drmem_info->n_lmbs - 1]; lmb++)`. When drmem_info->lmbs
> is NULL, the loop would iterate through the whole address range if it
> weren't stopped by the NULL pointer dereference on the next line.
>
> This patch aligns for_each_drmem_lmb and for_each_drmem_lmb_in_range macro
> behavior with the common C semantics, where the end marker does not belong
> to the scanned range, and alters get_lmb_range() semantics. As a side
> effect, the wraparound observed in the crash is prevented.
>
> Fixes: 6c6ea53725b3 ("powerpc/mm: Separate ibm, dynamic-memory data from DT format")
> Cc: Michal Suchanek <[email protected]>
> Cc: [email protected]
> Signed-off-by: Libor Pechacek <[email protected]>
> Signed-off-by: Michal Suchanek <[email protected]>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/a83836dbc53e96f13fec248ecc201d18e1e3111d
cheers