Linux-SCSI Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [PATCH] scsi/megaraid: Prevent kernel-infoleak in kioc_to_mimd()
@ 2020-07-27 21:02 Peilin Ye
  2020-07-28  8:41 ` Dan Carpenter
  0 siblings, 1 reply; 4+ messages in thread
From: Peilin Ye @ 2020-07-27 21:02 UTC (permalink / raw)
  To: Kashyap Desai, Sumit Saxena
  Cc: Peilin Ye, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	Shivasharan S, James E.J. Bottomley, Martin K. Petersen,
	linux-kernel-mentees, megaraidlinux.pdl, linux-scsi,
	linux-kernel

hinfo_to_cinfo() does no operation on `cinfo` when `hinfo` is NULL,
causing kioc_to_mimd() to copy uninitialized stack memory to userspace.
Fix it by initializing `cinfo` with memset().

Cc: stable@vger.kernel.org
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
 drivers/scsi/megaraid/megaraid_mm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/megaraid/megaraid_mm.c b/drivers/scsi/megaraid/megaraid_mm.c
index 8df53446641a..9df0e6b253a8 100644
--- a/drivers/scsi/megaraid/megaraid_mm.c
+++ b/drivers/scsi/megaraid/megaraid_mm.c
@@ -816,6 +816,8 @@ kioc_to_mimd(uioc_t *kioc, mimd_t __user *mimd)
 
 		case MEGAIOC_QADAPINFO:
 
+			memset(&cinfo, 0, sizeof(cinfo));
+
 			hinfo = (mraid_hba_info_t *)(unsigned long)
 					kioc->buf_vaddr;
 
-- 
2.25.1


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

* Re: [Linux-kernel-mentees] [PATCH] scsi/megaraid: Prevent kernel-infoleak in kioc_to_mimd()
  2020-07-27 21:02 [Linux-kernel-mentees] [PATCH] scsi/megaraid: Prevent kernel-infoleak in kioc_to_mimd() Peilin Ye
@ 2020-07-28  8:41 ` Dan Carpenter
  2020-07-28  8:57   ` Dan Carpenter
  2020-07-28 10:57   ` Peilin Ye
  0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-07-28  8:41 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Kashyap Desai, Sumit Saxena, Arnd Bergmann, Greg Kroah-Hartman,
	Shivasharan S, James E.J. Bottomley, Martin K. Petersen,
	linux-kernel-mentees, megaraidlinux.pdl, linux-scsi,
	linux-kernel

On Mon, Jul 27, 2020 at 05:02:35PM -0400, Peilin Ye wrote:
> hinfo_to_cinfo() does no operation on `cinfo` when `hinfo` is NULL,
> causing kioc_to_mimd() to copy uninitialized stack memory to userspace.
> Fix it by initializing `cinfo` with memset().

But "hinfo" can't be NULL so this patch isn't required.  It's a bit
hard for Smatch to follow the code.

We know that "opcode" is 82 so the buffer is allocated by mimd_to_kioc()
-> mraid_mm_attach_buf().

Generally, don't silence static checker warnings unless it makes the
code more readable.  It's the checker writer's job to fix their own code.
In this case, that's me, but parsing the code is quite complicated and I
don't have a plan for how to fix it.

regards,
dan carpenter


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

* Re: [Linux-kernel-mentees] [PATCH] scsi/megaraid: Prevent kernel-infoleak in kioc_to_mimd()
  2020-07-28  8:41 ` Dan Carpenter
@ 2020-07-28  8:57   ` Dan Carpenter
  2020-07-28 10:57   ` Peilin Ye
  1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-07-28  8:57 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Kashyap Desai, Sumit Saxena, Arnd Bergmann, Greg Kroah-Hartman,
	Shivasharan S, James E.J. Bottomley, Martin K. Petersen,
	linux-kernel-mentees, megaraidlinux.pdl, linux-scsi,
	linux-kernel

On Tue, Jul 28, 2020 at 11:41:37AM +0300, Dan Carpenter wrote:
> Generally, don't silence static checker warnings unless it makes the
> code more readable.  It's the checker writer's job to fix their own code.
> In this case, that's me, but parsing the code is quite complicated and I
> don't have a plan for how to fix it.

Actually, looking at this some more, it's not so complicated.  By this
time next year hopefully this warning will be silenced.

regards,
dan carpenter


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

* Re: [Linux-kernel-mentees] [PATCH] scsi/megaraid: Prevent kernel-infoleak in kioc_to_mimd()
  2020-07-28  8:41 ` Dan Carpenter
  2020-07-28  8:57   ` Dan Carpenter
@ 2020-07-28 10:57   ` Peilin Ye
  1 sibling, 0 replies; 4+ messages in thread
From: Peilin Ye @ 2020-07-28 10:57 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Kashyap Desai, Sumit Saxena, Arnd Bergmann, Greg Kroah-Hartman,
	Shivasharan S, James E.J. Bottomley, Martin K. Petersen,
	linux-kernel-mentees, megaraidlinux.pdl, linux-scsi,
	linux-kernel

On Tue, Jul 28, 2020 at 11:41:37AM +0300, Dan Carpenter wrote:
> On Mon, Jul 27, 2020 at 05:02:35PM -0400, Peilin Ye wrote:
> > hinfo_to_cinfo() does no operation on `cinfo` when `hinfo` is NULL,
> > causing kioc_to_mimd() to copy uninitialized stack memory to userspace.
> > Fix it by initializing `cinfo` with memset().
> 
> But "hinfo" can't be NULL so this patch isn't required.  It's a bit
> hard for Smatch to follow the code.
> 
> We know that "opcode" is 82 so the buffer is allocated by mimd_to_kioc()
> -> mraid_mm_attach_buf().

You are right. mraid_mm_ioctl() returns -ENOMEM and never reaches
kioc_to_mimd() if mraid_mm_attach_buf() failed to get a buffer, so
`hinfo` can never be NULL for kioc_to_mimd().

Next time I will trace the data flow more carefully. Thank you for
pointing this out!

Peilin Ye

> Generally, don't silence static checker warnings unless it makes the
> code more readable.  It's the checker writer's job to fix their own code.
> In this case, that's me, but parsing the code is quite complicated and I
> don't have a plan for how to fix it.
> 
> regards,
> dan carpenter
> 

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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-27 21:02 [Linux-kernel-mentees] [PATCH] scsi/megaraid: Prevent kernel-infoleak in kioc_to_mimd() Peilin Ye
2020-07-28  8:41 ` Dan Carpenter
2020-07-28  8:57   ` Dan Carpenter
2020-07-28 10:57   ` Peilin Ye

Linux-SCSI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-scsi/0 linux-scsi/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-scsi linux-scsi/ https://lore.kernel.org/linux-scsi \
		linux-scsi@vger.kernel.org
	public-inbox-index linux-scsi

Example config snippet for mirrors

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


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