Re: [PATCH v2] nvdimm: Avoid race between probe and reading device attributes
Hello Dan, Dan Williams writes: > > I see why this works, but I think the bug is in > available_slots_show(). It is a bug for a sysfs attribute to reference > driver-data without synchronizing against bind. So it should be > possible for probe set that pointer whenever it wants. In other words > this fix (forgive the whitespace damage from pasting). Ah, that makes sense! I see the new patch and all is good AFAICT. -- Thank you, Richard. ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v2] nvdimm: Avoid race between probe and reading device attributes
Yikes, sorry this languished so long, comments below:
On Mon, Jun 15, 2020 at 12:48 AM Richard Palethorpe
wrote:
>
> It is possible to cause a division error and use-after-free by querying the
> nmem device before the driver data is fully initialised in nvdimm_probe. E.g
> by doing
>
> (while true; do
> cat /sys/bus/nd/devices/nmem*/available_slots 2>&1 > /dev/null
> done) &
>
> while true; do
> for i in $(seq 0 4); do
> echo nmem$i > /sys/bus/nd/drivers/nvdimm/bind
> done
> for i in $(seq 0 4); do
> echo nmem$i > /sys/bus/nd/drivers/nvdimm/unbind
> done
> done
>
> On 5.7-rc3 this causes:
>
> [ 12.711578] divide error: [#1] SMP KASAN PTI
> [ 12.714857] RIP: 0010:nd_label_nfree+0x134/0x1a0 [libnvdimm]
[..]
> [ 12.725308] CR2: 7fd16f1ec000 CR3: 64322006 CR4:
> 00160ef0
> [ 12.726268] Call Trace:
> [ 12.726633] available_slots_show+0x4e/0x120 [libnvdimm]
> [ 12.727380] dev_attr_show+0x42/0x80
> [ 12.727891] ? memset+0x20/0x40
> [ 12.728341] sysfs_kf_seq_show+0x218/0x410
> [ 12.728923] seq_read+0x389/0xe10
> [ 12.729415] vfs_read+0x101/0x2d0
> [ 12.729891] ksys_read+0xf9/0x1d0
> [ 12.730361] ? kernel_write+0x120/0x120
> [ 12.730915] do_syscall_64+0x95/0x4a0
> [ 12.731435] entry_SYSCALL_64_after_hwframe+0x49/0xb3
[..]
> Fixes: 4d88a97aa9e8 ("libnvdimm, nvdimm: dimm driver and base libnvdimm
> device-driver infrastructure")
> Cc: Dan Williams
> Cc: Vishal Verma
> Cc: Dave Jiang
> Cc: Ira Weiny
> Cc: linux-nvdimm@lists.01.org
> Cc: linux-ker...@vger.kernel.org
> Cc: Coly Li
> Signed-off-by: Richard Palethorpe
> ---
>
> V2:
> + Reviewed by Coly and removed unecessary lock
>
> drivers/nvdimm/dimm.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
> index 7d4ddc4d9322..3d3988e1d9a0 100644
> --- a/drivers/nvdimm/dimm.c
> +++ b/drivers/nvdimm/dimm.c
> @@ -43,7 +43,6 @@ static int nvdimm_probe(struct device *dev)
> if (!ndd)
> return -ENOMEM;
>
> - dev_set_drvdata(dev, ndd);
> ndd->dpa.name = dev_name(dev);
> ndd->ns_current = -1;
> ndd->ns_next = -1;
> @@ -106,6 +105,8 @@ static int nvdimm_probe(struct device *dev)
> if (rc)
> goto err;
>
> + dev_set_drvdata(dev, ndd);
> +
I see why this works, but I think the bug is in
available_slots_show(). It is a bug for a sysfs attribute to reference
driver-data without synchronizing against bind. So it should be
possible for probe set that pointer whenever it wants. In other words
this fix (forgive the whitespace damage from pasting).
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index b59032e0859b..e68b17bc7aab 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -335,10 +335,8 @@ static ssize_t state_show(struct device *dev,
struct device_attribute *attr,
}
static DEVICE_ATTR_RO(state);
-static ssize_t available_slots_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t __available_slots_show(struct nvdimm_drvdata *ndd, char *buf)
{
- struct nvdimm_drvdata *ndd = dev_get_drvdata(dev);
ssize_t rc;
u32 nfree;
@@ -356,6 +354,18 @@ static ssize_t available_slots_show(struct device *dev,
nvdimm_bus_unlock(dev);
return rc;
}
+
+static ssize_t available_slots_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ ssize_t rc;
+
+ nd_device_lock(dev);
+ rc = __available_slots_show(dev_get_drvdata(dev), buf);
+ nd_device_unlock(dev);
+
+ return rc;
+}
static DEVICE_ATTR_RO(available_slots);
__weak ssize_t security_show(struct device *dev,
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v2] nvdimm: Avoid race between probe and reading device attributes
Ping? On Mon, Jun 15, 2020 at 08:47:23AM +0100, Richard Palethorpe wrote: > It is possible to cause a division error and use-after-free by querying the > nmem device before the driver data is fully initialised in nvdimm_probe. E.g > by doing > > (while true; do > cat /sys/bus/nd/devices/nmem*/available_slots 2>&1 > /dev/null > done) & > > while true; do > for i in $(seq 0 4); do >echo nmem$i > /sys/bus/nd/drivers/nvdimm/bind > done > for i in $(seq 0 4); do >echo nmem$i > /sys/bus/nd/drivers/nvdimm/unbind > done > done > > On 5.7-rc3 this causes: > > [ 12.711578] divide error: [#1] SMP KASAN PTI > [ 12.712321] CPU: 0 PID: 231 Comm: cat Not tainted 5.7.0-rc3 #48 > [ 12.713188] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014 > [ 12.714857] RIP: 0010:nd_label_nfree+0x134/0x1a0 [libnvdimm] > [ 12.715772] Code: ba 00 00 00 00 00 fc ff df 48 89 f9 48 c1 e9 03 0f b6 14 > 11 84 d2 74 05 80 fa 03 7e 52 8b 73 08 31 d2 89 c1 48 83 c4 08 5b 5d f6 > 31 d2 41 5c 83 c0 07 c1 e8 03 48 8d 84 00 8e 02 00 00 25 00 > [ 12.718311] RSP: 0018:c946fd08 EFLAGS: 00010282 > [ 12.719030] RAX: RBX: c0073aa0 RCX: > > [ 12.720005] RDX: RSI: RDI: > 888060931808 > [ 12.720970] RBP: 88806609d018 R08: 0001 R09: > ed100cc0a2b1 > [ 12.721889] R10: 888066051587 R11: ed100cc0a2b0 R12: > 888060931800 > [ 12.722744] R13: 888064362000 R14: 88806609d018 R15: > 8b1a2520 > [ 12.723602] FS: 7fd16f3d5580() GS:88806b40() > knlGS: > [ 12.724600] CS: 0010 DS: ES: CR0: 80050033 > [ 12.725308] CR2: 7fd16f1ec000 CR3: 64322006 CR4: > 00160ef0 > [ 12.726268] Call Trace: > [ 12.726633] available_slots_show+0x4e/0x120 [libnvdimm] > [ 12.727380] dev_attr_show+0x42/0x80 > [ 12.727891] ? memset+0x20/0x40 > [ 12.728341] sysfs_kf_seq_show+0x218/0x410 > [ 12.728923] seq_read+0x389/0xe10 > [ 12.729415] vfs_read+0x101/0x2d0 > [ 12.729891] ksys_read+0xf9/0x1d0 > [ 12.730361] ? kernel_write+0x120/0x120 > [ 12.730915] do_syscall_64+0x95/0x4a0 > [ 12.731435] entry_SYSCALL_64_after_hwframe+0x49/0xb3 > [ 12.732163] RIP: 0033:0x7fd16f2fe4be > [ 12.732685] Code: c0 e9 c6 fe ff ff 50 48 8d 3d 2e 12 0a 00 e8 69 e9 01 00 > 66 0f 1f 84 00 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 0f 05 <48> 3d > 00 f0 ff ff 77 5a c3 66 0f 1f 84 00 00 00 00 00 48 83 ec 28 > [ 12.735207] RSP: 002b:7ffd3177b838 EFLAGS: 0246 ORIG_RAX: > > [ 12.736261] RAX: ffda RBX: 0002 RCX: > 7fd16f2fe4be > [ 12.737233] RDX: 0002 RSI: 7fd16f1ed000 RDI: > 0003 > [ 12.738203] RBP: 7fd16f1ed000 R08: 7fd16f1ec010 R09: > > [ 12.739172] R10: 7fd16f3f4f70 R11: 0246 R12: > 7ffd3177ce23 > [ 12.740144] R13: 0003 R14: 0002 R15: > 0002 > [ 12.741139] Modules linked in: nfit libnvdimm > [ 12.741783] ---[ end trace 99532e4b82410044 ]--- > [ 12.742452] RIP: 0010:nd_label_nfree+0x134/0x1a0 [libnvdimm] > [ 12.743167] Code: ba 00 00 00 00 00 fc ff df 48 89 f9 48 c1 e9 03 0f b6 14 > 11 84 d2 74 05 80 fa 03 7e 52 8b 73 08 31 d2 89 c1 48 83 c4 08 5b 5d f6 > 31 d2 41 5c 83 c0 07 c1 e8 03 48 8d 84 00 8e 02 00 00 25 00 > [ 12.745709] RSP: 0018:c946fd08 EFLAGS: 00010282 > [ 12.746340] RAX: RBX: c0073aa0 RCX: > > [ 12.747209] RDX: RSI: RDI: > 888060931808 > [ 12.748081] RBP: 88806609d018 R08: 0001 R09: > ed100cc0a2b1 > [ 12.748977] R10: 888066051587 R11: ed100cc0a2b0 R12: > 888060931800 > [ 12.749849] R13: 888064362000 R14: 88806609d018 R15: > 8b1a2520 > [ 12.750729] FS: 7fd16f3d5580() GS:88806b40() > knlGS: > [ 12.751708] CS: 0010 DS: ES: CR0: 80050033 > [ 12.752441] CR2: 7fd16f1ec000 CR3: 64322006 CR4: > 00160ef0 > [ 12.821357] > == > [ 12.822284] BUG: KASAN: use-after-free in __mutex_lock+0x111c/0x11a0 > [ 12.823084] Read of size 4 at addr 888065c26238 by task reproducer/218 > [ 12.823968] > [ 12.824183] CPU: 2 PID: 218 Comm: reproducer Tainted: G D > 5.7.0-rc3 #48 > [ 12.825167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014 > [ 12.826595] Call Trace: > [ 12.826926] dump_stack+0x97/0xe0 > [ 12.827362] print_address_description.constprop.0+0x1b/0x210 > [ 12.828111] ? __mutex_lock+0x111c/0x11a0 > [ 12.828645]
Re: [PATCH v2] nvdimm: Avoid race between probe and reading device attributes
On 2020/6/15 15:47, Richard Palethorpe wrote:
> It is possible to cause a division error and use-after-free by querying the
> nmem device before the driver data is fully initialised in nvdimm_probe. E.g
> by doing
>
> (while true; do
> cat /sys/bus/nd/devices/nmem*/available_slots 2>&1 > /dev/null
> done) &
>
> while true; do
> for i in $(seq 0 4); do
>echo nmem$i > /sys/bus/nd/drivers/nvdimm/bind
> done
> for i in $(seq 0 4); do
>echo nmem$i > /sys/bus/nd/drivers/nvdimm/unbind
> done
> done
>
> On 5.7-rc3 this causes:
[snipped]
>
> This can be prevented by setting the driver data after initialisation is
> complete.
>
> Fixes: 4d88a97aa9e8 ("libnvdimm, nvdimm: dimm driver and base libnvdimm
> device-driver infrastructure")
> Cc: Dan Williams
> Cc: Vishal Verma
> Cc: Dave Jiang
> Cc: Ira Weiny
> Cc: linux-nvdimm@lists.01.org
> Cc: linux-ker...@vger.kernel.org
> Cc: Coly Li
> Signed-off-by: Richard Palethorpe
Reviewed-by: Coly Li
Thanks.
Coly Li
> ---
>
> V2:
> + Reviewed by Coly and removed unecessary lock
>
> drivers/nvdimm/dimm.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
> index 7d4ddc4d9322..3d3988e1d9a0 100644
> --- a/drivers/nvdimm/dimm.c
> +++ b/drivers/nvdimm/dimm.c
> @@ -43,7 +43,6 @@ static int nvdimm_probe(struct device *dev)
> if (!ndd)
> return -ENOMEM;
>
> - dev_set_drvdata(dev, ndd);
> ndd->dpa.name = dev_name(dev);
> ndd->ns_current = -1;
> ndd->ns_next = -1;
> @@ -106,6 +105,8 @@ static int nvdimm_probe(struct device *dev)
> if (rc)
> goto err;
>
> + dev_set_drvdata(dev, ndd);
> +
> return 0;
>
> err:
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
