Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] EDAC: Fix global-out-of-bounds write when setting edac_mc_poll_msec
@ 2019-06-26  5:40 Eiichi Tsukata
  2019-06-27 17:11 ` James Morse
  0 siblings, 1 reply; 3+ messages in thread
From: Eiichi Tsukata @ 2019-06-26  5:40 UTC (permalink / raw)
  To: bp, mchehab, james.morse, linux-edac, linux-kernel; +Cc: Eiichi Tsukata

Commit 9da21b1509d8 ("EDAC: Poll timeout cannot be zero, p2") assumes
edac_mc_poll_msec to be unsigned long, but the type of the variable still
remained as int. Setting edac_mc_poll_msec can trigger out-of-bounds
write.

Reproducer:

  # echo 1001 > /sys/module/edac_core/parameters/edac_mc_poll_msec

KASAN report:

  BUG: KASAN: global-out-of-bounds in edac_set_poll_msec+0x140/0x150
  Write of size 8 at addr ffffffffb91b2d00 by task bash/1996

  CPU: 1 PID: 1996 Comm: bash Not tainted 5.2.0-rc6+ #23
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-2.fc30 04/01/2014
  Call Trace:
   dump_stack+0xca/0x13e
   print_address_description.cold+0x5/0x246
   __kasan_report.cold+0x75/0x9a
   ? edac_set_poll_msec+0x140/0x150
   kasan_report+0xe/0x20
   edac_set_poll_msec+0x140/0x150
   ? dimmdev_location_show+0x30/0x30
   ? vfs_lock_file+0xe0/0xe0
   ? _raw_spin_lock+0x87/0xe0
   param_attr_store+0x1b5/0x310
   ? param_array_set+0x4f0/0x4f0
   module_attr_store+0x58/0x80
   ? module_attr_show+0x80/0x80
   sysfs_kf_write+0x13d/0x1a0
   kernfs_fop_write+0x2bc/0x460
   ? sysfs_kf_bin_read+0x270/0x270
   ? kernfs_notify+0x1f0/0x1f0
   __vfs_write+0x81/0x100
   vfs_write+0x1e1/0x560
   ksys_write+0x126/0x250
   ? __ia32_sys_read+0xb0/0xb0
   ? do_syscall_64+0x1f/0x390
   do_syscall_64+0xc1/0x390
   entry_SYSCALL_64_after_hwframe+0x49/0xbe
  RIP: 0033:0x7fa7caa5e970
  Code: 73 01 c3 48 8b 0d 28 d5 2b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 83 3d 99 2d 2c 00 00 75 10 b8 01 00 00 00 04
  RSP: 002b:00007fff6acfdfe8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
  RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00007fa7caa5e970
  RDX: 0000000000000005 RSI: 0000000000e95c08 RDI: 0000000000000001
  RBP: 0000000000e95c08 R08: 00007fa7cad1e760 R09: 00007fa7cb36a700
  R10: 0000000000000073 R11: 0000000000000246 R12: 0000000000000005
  R13: 0000000000000001 R14: 00007fa7cad1d600 R15: 0000000000000005

  The buggy address belongs to the variable:
   edac_mc_poll_msec+0x0/0x40

  Memory state around the buggy address:
   ffffffffb91b2c00: 00 00 00 00 fa fa fa fa 00 00 00 00 fa fa fa fa
   ffffffffb91b2c80: 00 00 00 00 fa fa fa fa 00 00 00 00 fa fa fa fa
  >ffffffffb91b2d00: 04 fa fa fa fa fa fa fa 04 fa fa fa fa fa fa fa
                     ^
   ffffffffb91b2d80: 04 fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
   ffffffffb91b2e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Fix it by changing the type of edac_mc_poll_msec to unsigned int.
The reason why this patch adopts unsigned int rather than unsigned long
is msecs_to_jiffies() assumes arg to be unsigned int. We can avoid
integer conversion bugs and unsigned int will be large enough for
edac_mc_poll_msec.

Fixes: 9da21b1509d8 ("EDAC: Poll timeout cannot be zero, p2")
Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
---
 drivers/edac/edac_mc_sysfs.c | 16 ++++++++--------
 drivers/edac/edac_module.h   |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 7c01e1cc030c..4386ea4b9b5a 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -26,7 +26,7 @@
 static int edac_mc_log_ue = 1;
 static int edac_mc_log_ce = 1;
 static int edac_mc_panic_on_ue;
-static int edac_mc_poll_msec = 1000;
+static unsigned int edac_mc_poll_msec = 1000;
 
 /* Getter functions for above */
 int edac_mc_get_log_ue(void)
@@ -45,30 +45,30 @@ int edac_mc_get_panic_on_ue(void)
 }
 
 /* this is temporary */
-int edac_mc_get_poll_msec(void)
+unsigned int edac_mc_get_poll_msec(void)
 {
 	return edac_mc_poll_msec;
 }
 
 static int edac_set_poll_msec(const char *val, const struct kernel_param *kp)
 {
-	unsigned long l;
+	unsigned int i;
 	int ret;
 
 	if (!val)
 		return -EINVAL;
 
-	ret = kstrtoul(val, 0, &l);
+	ret = kstrtouint(val, 0, &i);
 	if (ret)
 		return ret;
 
-	if (l < 1000)
+	if (i < 1000)
 		return -EINVAL;
 
-	*((unsigned long *)kp->arg) = l;
+	*((unsigned int *)kp->arg) = i;
 
 	/* notify edac_mc engine to reset the poll period */
-	edac_mc_reset_delay_period(l);
+	edac_mc_reset_delay_period(i);
 
 	return 0;
 }
@@ -82,7 +82,7 @@ MODULE_PARM_DESC(edac_mc_log_ue,
 module_param(edac_mc_log_ce, int, 0644);
 MODULE_PARM_DESC(edac_mc_log_ce,
 		 "Log correctable error to console: 0=off 1=on");
-module_param_call(edac_mc_poll_msec, edac_set_poll_msec, param_get_int,
+module_param_call(edac_mc_poll_msec, edac_set_poll_msec, param_get_uint,
 		  &edac_mc_poll_msec, 0644);
 MODULE_PARM_DESC(edac_mc_poll_msec, "Polling period in milliseconds");
 
diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h
index bc4b806dc9cc..b2f59ee76c22 100644
--- a/drivers/edac/edac_module.h
+++ b/drivers/edac/edac_module.h
@@ -36,7 +36,7 @@ extern int edac_mc_get_log_ue(void);
 extern int edac_mc_get_log_ce(void);
 extern int edac_mc_get_panic_on_ue(void);
 extern int edac_get_poll_msec(void);
-extern int edac_mc_get_poll_msec(void);
+extern unsigned int edac_mc_get_poll_msec(void);
 
 unsigned edac_dimm_info_location(struct dimm_info *dimm, char *buf,
 				 unsigned len);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] EDAC: Fix global-out-of-bounds write when setting edac_mc_poll_msec
  2019-06-26  5:40 [PATCH] EDAC: Fix global-out-of-bounds write when setting edac_mc_poll_msec Eiichi Tsukata
@ 2019-06-27 17:11 ` James Morse
  2019-06-27 17:36   ` Luck, Tony
  0 siblings, 1 reply; 3+ messages in thread
From: James Morse @ 2019-06-27 17:11 UTC (permalink / raw)
  To: Eiichi Tsukata; +Cc: bp, mchehab, linux-edac, linux-kernel, Tony Luck

Hello,

(CC: +Tony Luck.
 Original Patch: lore.kernel.org/r/20190626054011.30044-1-devel@etsukata.com )

On 26/06/2019 06:40, Eiichi Tsukata wrote:
> Commit 9da21b1509d8 ("EDAC: Poll timeout cannot be zero, p2") assumes
> edac_mc_poll_msec to be unsigned long, but the type of the variable still
> remained as int. Setting edac_mc_poll_msec can trigger out-of-bounds
> write.

Thanks for catching this!


> Fix it by changing the type of edac_mc_poll_msec to unsigned int.

This means reverting more of 9da21b1509d8, but it also fixes signed/unsigned issues:
| root@debian-guest:/sys/module/edac_core/parameters# echo 4294967295 >  edac_mc_poll_msec
| root@debian-guest:/sys/module/edac_core/parameters# cat edac_mc_poll_msec
| -1
| root@debian-guest:/sys/module/edac_core/parameters# echo -1 > edac_mc_poll_msec
| -bash: echo: write error: Invalid argument


> The reason why this patch adopts unsigned int rather than unsigned long
> is msecs_to_jiffies() assumes arg to be unsigned int.

Ah, so the range is limited anyway.

It looks like it was switched to long to be consistent with edac_mc_workq_setup(), which
has since been removed in preference to msecs_to_jiffies().


Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] EDAC: Fix global-out-of-bounds write when setting edac_mc_poll_msec
  2019-06-27 17:11 ` James Morse
@ 2019-06-27 17:36   ` Luck, Tony
  0 siblings, 0 replies; 3+ messages in thread
From: Luck, Tony @ 2019-06-27 17:36 UTC (permalink / raw)
  To: James Morse; +Cc: Eiichi Tsukata, bp, mchehab, linux-edac, linux-kernel

On Thu, Jun 27, 2019 at 06:11:18PM +0100, James Morse wrote:
> Hello,
> 
> (CC: +Tony Luck.
>  Original Patch: lore.kernel.org/r/20190626054011.30044-1-devel@etsukata.com )

Heh: My mail agent "helpfully" made that clickable, but as a "mailto:"
URL rather than an https: one!
> 
> On 26/06/2019 06:40, Eiichi Tsukata wrote:
> > Commit 9da21b1509d8 ("EDAC: Poll timeout cannot be zero, p2") assumes
> > edac_mc_poll_msec to be unsigned long, but the type of the variable still
> > remained as int. Setting edac_mc_poll_msec can trigger out-of-bounds
> > write.
> 
> Thanks for catching this!

Ditto & likewise.
> 
> 
> > Fix it by changing the type of edac_mc_poll_msec to unsigned int.
> 
> This means reverting more of 9da21b1509d8, but it also fixes signed/unsigned issues:
> | root@debian-guest:/sys/module/edac_core/parameters# echo 4294967295 >  edac_mc_poll_msec
> | root@debian-guest:/sys/module/edac_core/parameters# cat edac_mc_poll_msec
> | -1
> | root@debian-guest:/sys/module/edac_core/parameters# echo -1 > edac_mc_poll_msec
> | -bash: echo: write error: Invalid argument
> 
> 
> > The reason why this patch adopts unsigned int rather than unsigned long
> > is msecs_to_jiffies() assumes arg to be unsigned int.
> 
> Ah, so the range is limited anyway.
> 
> It looks like it was switched to long to be consistent with edac_mc_workq_setup(), which
> has since been removed in preference to msecs_to_jiffies().
> 
> 
> Reviewed-by: James Morse <james.morse@arm.com>

Applied. (Boris left me in charge of EDAC bits for the
next few weeks). It will show up in the for_5.3 branch
of:
  git://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git
after test builds complete.

-Tony

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-26  5:40 [PATCH] EDAC: Fix global-out-of-bounds write when setting edac_mc_poll_msec Eiichi Tsukata
2019-06-27 17:11 ` James Morse
2019-06-27 17:36   ` Luck, Tony

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org linux-edac@archiver.kernel.org
	public-inbox-index linux-edac


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/ public-inbox