From owner-freebsd-arch@FreeBSD.ORG Fri Jun 20 18:09:36 2003 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 157C437B401; Fri, 20 Jun 2003 18:09:36 -0700 (PDT) Received: from mail.cyberonic.com (mail.cyberonic.com [4.17.179.4]) by mx1.FreeBSD.org (Postfix) with ESMTP id E491B43F3F; Fri, 20 Jun 2003 18:09:34 -0700 (PDT) (envelope-from jmg@hydrogen.funkthat.com) Received: from hydrogen.funkthat.com (node-40244c0a.sfo.onnet.us.uu.net [64.36.76.10]) by mail.cyberonic.com (8.12.8/8.12.5) with ESMTP id h5L1aVMo017653; Fri, 20 Jun 2003 21:36:32 -0400 Received: (from jmg@localhost) by hydrogen.funkthat.com (8.12.9/8.11.6) id h5L1A2B5030917; Fri, 20 Jun 2003 18:10:02 -0700 (PDT) (envelope-from jmg) Date: Fri, 20 Jun 2003 18:10:02 -0700 From: John-Mark Gurney To: Bruce Evans , Robert Watson , arch@freebsd.org Message-ID: <20030621011002.GG15336@funkthat.com> Mail-Followup-To: Bruce Evans , Robert Watson , arch@freebsd.org References: <20030617120956.N30677@gamplex.bde.org> <20030617052917.GF73854@funkthat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="ReaqsoxgOBHFXBhH" Content-Disposition: inline In-Reply-To: <20030617052917.GF73854@funkthat.com> User-Agent: Mutt/1.4.1i X-Operating-System: FreeBSD 4.2-RELEASE i386 X-PGP-Fingerprint: B7 EC EF F8 AE ED A7 31 96 7A 22 B3 D8 56 36 F4 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html Subject: Re: make /dev/pci really readable X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: John-Mark Gurney List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Jun 2003 01:09:36 -0000 --ReaqsoxgOBHFXBhH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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." --ReaqsoxgOBHFXBhH Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="pci_user2.patch" 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; --ReaqsoxgOBHFXBhH--