make /dev/pci really readable

[John-Mark Gurney]

John-Mark Gurney wrote this message on Mon, Jun 16, 2003 at 22:29 -0700:
> Bruce Evans wrote this message on Tue, Jun 17, 2003 at 12:36 +1000:
> > On Mon, 16 Jun 2003, Robert Watson wrote:
> > > It looks like (although I haven't tried), user processes can
> > > also cause the kernel to allocate unlimited amounts of kernel memory,
> > > which is another bit we probably need to tighten down.
> > 
> > Much more serious.
> 
> Yep, the pattern_buf is allocated, and in some cases a berak happens
> w/o freeing it.  So there is a memory leak her. Will be fixed soon.

Ok, I think I have a good patch.  It's attached.  Fixes the memory
leak.  I have also fix the pci manpage to talk about the errors, but
it isn't included in the patch.

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."
-------------- next part --------------
Index: pci_user.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/pci/pci_user.c,v
retrieving revision 1.9
diff -u -r1.9 pci_user.c
--- pci_user.c	2003/03/03 12:15:44	1.9
+++ pci_user.c	2003/06/21 00:29:48
@@ -176,9 +176,14 @@
 	const char *name;
 	int error;
 
-	if (!(flag & FWRITE))
+	if (!(flag & FWRITE) && cmd != PCIOCGETCONF)
 		return EPERM;
 
+	/* make sure register is in bounds and aligned */
+	if (cmd == PCIOCREAD || cmd == PCIOCWRITE)
+		if (io->pi_reg < 0 || io->pi_reg + io->pi_width > PCI_REGMAX ||
+		    io->pi_reg & (io->pi_width - 1))
+			error = EINVAL;
 
 	switch(cmd) {
 	case PCIOCGETCONF:
@@ -197,15 +202,6 @@
 		dinfo = NULL;
 
 		/*
-		 * Hopefully the user won't pass in a null pointer, but it
-		 * can't hurt to check.
-		 */
-		if (cio == NULL) {
-			error = EINVAL;
-			break;
-		}
-
-		/*
 		 * If the user specified an offset into the device list,
 		 * but the list has changed since they last called this
 		 * ioctl, tell them that the list has changed.  They will
@@ -272,42 +268,22 @@
 			    sizeof(struct pci_match_conf)) != cio->pat_buf_len){
 				/* The user made a mistake, return an error*/
 				cio->status = PCI_GETCONF_ERROR;
-				printf("pci_ioctl: pat_buf_len %d != "
-				       "num_patterns (%d) * sizeof(struct "
-				       "pci_match_conf) (%d)\npci_ioctl: "
-				       "pat_buf_len should be = %d\n",
-				       cio->pat_buf_len, cio->num_patterns,
-				       (int)sizeof(struct pci_match_conf),
-				       (int)sizeof(struct pci_match_conf) * 
-				       cio->num_patterns);
-				printf("pci_ioctl: do your headers match your "
-				       "kernel?\n");
 				cio->num_matches = 0;
 				error = EINVAL;
 				break;
 			}
 
 			/*
-			 * Check the user's buffer to make sure it's readable.
-			 */
-			if (!useracc((caddr_t)cio->patterns,
-				    cio->pat_buf_len, VM_PROT_READ)) {
-				printf("pci_ioctl: pattern buffer %p, "
-				       "length %u isn't user accessible for"
-				       " READ\n", cio->patterns,
-				       cio->pat_buf_len);
-				error = EACCES;
-				break;
-			}
-			/*
 			 * Allocate a buffer to hold the patterns.
 			 */
 			pattern_buf = malloc(cio->pat_buf_len, M_TEMP,
 					     M_WAITOK);
 			error = copyin(cio->patterns, pattern_buf,
 				       cio->pat_buf_len);
-			if (error != 0)
-				break;
+			if (error != 0) {
+				error = EINVAL;
+				goto getconfexit;
+			}
 			num_patterns = cio->num_patterns;
 
 		} else if ((cio->num_patterns > 0)
@@ -317,32 +293,19 @@
 			 */
 			cio->status = PCI_GETCONF_ERROR;
 			cio->num_matches = 0;
-			printf("pci_ioctl: invalid GETCONF arguments\n");
 			error = EINVAL;
 			break;
 		} else
 			pattern_buf = NULL;
 
 		/*
-		 * Make sure we can write to the match buffer.
-		 */
-		if (!useracc((caddr_t)cio->matches,
-			     cio->match_buf_len, VM_PROT_WRITE)) {
-			printf("pci_ioctl: match buffer %p, length %u "
-			       "isn't user accessible for WRITE\n",
-			       cio->matches, cio->match_buf_len);
-			error = EACCES;
-			break;
-		}
-
-		/*
 		 * Go through the list of devices and copy out the devices
 		 * that match the user's criteria.
 		 */
 		for (cio->num_matches = 0, error = 0, i = 0,
 		     dinfo = STAILQ_FIRST(devlist_head);
 		     (dinfo != NULL) && (cio->num_matches < ionum)
-		     && (error == 0) && (i < pci_numdevs);
+		     && (error == 0) && (i < pci_numdevs) && (dinfo != NULL);
 		     dinfo = STAILQ_NEXT(dinfo, pci_links), i++) {
 
 			if (i < cio->offset)
@@ -375,10 +338,12 @@
 				if (cio->num_matches >= ionum)
 					break;
 
-				error = copyout(&dinfo->conf,
-					        &cio->matches[cio->num_matches],
-						sizeof(struct pci_conf));
-				cio->num_matches++;
+				/* only if can copy it out do we count it */
+				if (!(error = copyout(&dinfo->conf,
+				    &cio->matches[cio->num_matches],
+				    sizeof(struct pci_conf)))) {
+					cio->num_matches++;
+				}
 			}
 		}
 
@@ -405,6 +370,7 @@
 		else
 			cio->status = PCI_GETCONF_MORE_DEVS;
 
+getconfexit:
 		if (pattern_buf != NULL)
 			free(pattern_buf, M_TEMP);
 
@@ -439,7 +405,7 @@
 			}
 			break;
 		default:
-			error = ENODEV;
+			error = EINVAL;
 			break;
 		}
 		break;
@@ -473,7 +439,7 @@
 			}
 			break;
 		default:
-			error = ENODEV;
+			error = EINVAL;
 			break;
 		}
 		break;