Bug 13745 - megaraid 1e08 driver rejected
Summary: megaraid 1e08 driver rejected
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Red Hat Linux
Classification: Retired
Component: kernel
Version: 7.1
Hardware: i386
OS: Linux
high
high
Target Milestone: ---
Assignee: Michael K. Johnson
QA Contact:
URL:
Whiteboard: Winston rc1
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2000-07-11 19:52 UTC by Matt Domsch
Modified: 2008-05-01 15:37 UTC (History)
1 user (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2000-08-09 17:03:17 UTC
Embargoed:


Attachments (Terms of Use)
megaraid.patch (5.12 KB, patch)
2000-08-05 01:16 UTC, Matt Domsch
no flags Details | Diff
megaraid-1.09.tgz (24.45 KB, application/octet-stream)
2000-08-08 03:06 UTC, Matt Domsch
no flags Details

Description Matt Domsch 2000-07-11 19:52:01 UTC
megaraid driver 1b08b and 1e08 (functionally identical) both have this 
same problem which caused Alan to reject the driver update.  AMI needs 
Alan's help to resolve this issue properly.


Rejected. This is completely unsafe

#if LINUX_VERSION_CODE > 0x020024
/*
 * usage of the function copy from user is used in case of data more than
 * 4KB.  This is used only with adapters which supports more than 8 logical
 * drives.  This feature is disabled on kernels earlier or same as 2.0.36  
 * as the uaccess.h file is not available with those kernels.
 */
   
  if (SCpnt->cmnd[0] == IOCTL_CMD_NEW) {
            /* use external data area for large xfers  */
     /* If cmnd[0] is set to IOCTL_CMD_NEW then *
      *   cmnd[4..7] = external user buffer     *
      *   cmnd[8..11] = length of buffer        *
      *                                         */
      u32 xfer_size = *((u32 *)&SCpnt->cmnd[8]);  
      user_area = *((char **)&SCpnt->cmnd[4]);    
      if (verify_area(VERIFY_READ, user_area, xfer_size)) {
          printk("megaraid: Got bad user address.\n");
          SCpnt->result = (DID_ERROR << 16);
          callDone (SCpnt);
          return NULL;
	}
      mega_build_user_sg(user_area, xfer_size, pScb, mbox);

  }
#endif

You cannot go around peeking into unlocked user pages. Nor can you assume
they are even reachable by PCI. You can scatter gather fine but use
get_free_page() and copy the data.


Alan

Comment 1 Glen Foster 2000-07-30 22:55:18 UTC
This defect is considered MUST-FIX for Winston Release-Candidate #1

... this bug needs some serious attention (based on Alan's comments), Doug. 
Please do what you can to take care of this defect ASAP.  I don't want to have
another "post-release AMI Mega-RAID" issue like we did in Zoot. :-)

Comment 2 Alan Cox 2000-07-31 00:56:15 UTC
I have talked with Peter about doing it the way it was done before and I think
we are both happy
on that and the nasty hack removed. Check with PeterJ


Comment 3 Matt Domsch 2000-08-05 01:04:21 UTC
Doug's second megaraid patch dereferences a NULL pointer somewhere when the 
driver loads.  He thought it might not be 100% ready, but more a proof-of-
concept.  I'm afraid I haven't had time to debug it properly, so if Michael or 
PeterJ can, that's great.


Comment 4 Matt Domsch 2000-08-05 01:16:17 UTC
Created attachment 2075 [details]
megaraid.patch

Comment 5 Matt Domsch 2000-08-05 01:17:02 UTC
Doug's comments on making the above patch:

OK, attached is a new version of the patch I sent last night.  It fixes the
fact that my first patch was really begging me not to leak 32K of kmalloc'ed
RAM with every ioctl (doh!!) and it also moves to per Scb semaphores so that
multiple ioctl commands won't step on each other.  Finally, I'm not a
semaphore expert, so should I be adding a wait queue to the semaphore init, or
does the kernel use a generic one in this case?  I think it just uses a
generic one and the right spot will get woken up when the up() happens, but I
could be wrong.  Anyway, the process I have in there should be sound.  The
main thing is that you can't hold the io_request_lock or the driver lock or
any other lock for that matter during either the down() or the copy_to_user(),
the rest of the time you should be locked.


Comment 6 Michael K. Johnson 2000-08-07 19:25:37 UTC
Giving this one back to you, Doug!  Thanks...

Comment 7 Matt Domsch 2000-08-07 19:33:36 UTC
AMI found the NULL pointer dereference bug that Doug had left in there.  The
rest of it looks great!  AMI and Dell are testing, and will resubmit ASAP.

Peter, when you're confident, can you release a 1.09 driver?
-Matt


Comment 8 Michael K. Johnson 2000-08-08 02:44:31 UTC
ASAP is putting it very lightly.  Peter, if you are
not confident enough to release a 1.09 driver, could
you please at least send the fix for the NULL pointer
deref for us to drop in for initial tests -- I need
it in a few *hours* to make RC1.

Comment 9 Matt Domsch 2000-08-08 03:06:21 UTC
Created attachment 2234 [details]
megaraid-1.09.tgz

Comment 10 Matt Domsch 2000-08-08 03:08:35 UTC
The above megaraid-1.09.tgz is exactly the 1b0b driver, + Doug's patch, + 
Manoj's fix to Doug's patch.  I'm sending this now because time is short.  I'm 
not precluding PeterJ from sending a "better" version.  We've tested this 
driver somewhat, and it certainly solves the NULL pointer dereference issue.

Comment 11 Michael K. Johnson 2000-08-08 04:05:29 UTC
Thanks for your quick response, Matt.  I'm in the
process of integrating the patch now.  I expect
to give you a kernel tomorrow for testing, and it
will have this version of the driver in it.

Comment 12 Michael K. Johnson 2000-08-08 13:37:28 UTC
OK, Matt, there is now a 2.2.16-19.1 kernel in your
private directory on gribble.  Please test.  Peter,
if you want a copy, I can give one to you, or you
can get it from Matt, whichever works best.

Comment 13 Michael K. Johnson 2000-08-09 16:56:26 UTC
Since the driver is integrated, I'm closing this.
Matt, I'm a bit confused about the megamgr program
bug that you reported earlier; want to open a new
bug report about that bug if it is a real bug?


Comment 14 Matt Domsch 2000-08-09 17:03:15 UTC
I don't know if it's a real bug yet or not, but I'll open a new issue if/when I 
can prove either way.


Note You need to log in before you can comment on or make changes to this bug.