Date: Mon, 16 Jun 2003 22:29:17 -0700 From: John-Mark Gurney <gurney_j@efn.org> To: Bruce Evans <bde@zeta.org.au> Cc: Robert Watson <rwatson@freebsd.org> Subject: Re: make /dev/pci really readable Message-ID: <20030617052917.GF73854@funkthat.com> In-Reply-To: <20030617120956.N30677@gamplex.bde.org> References: <Pine.NEB.3.96L.1030616155956.8726C-100000@fledge.watson.org> <20030617120956.N30677@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Bruce Evans wrote this message on Tue, Jun 17, 2003 at 12:36 +1000:
> On Mon, 16 Jun 2003, Robert Watson wrote:
>
> > On Mon, 16 Jun 2003, John-Mark Gurney wrote:
> >
> > > again, I just proposed -l, not -r to become user readable. I know that
> > > -r has problems. I've crashed the sparc box a number of times by
> > > specifing pciconf -r pci1:5:0 0x0:0xf.
>
> Yes, it seems that -l is fairly safe, and it wasn't the default just because
> no one wrote the small hack to change the permissions check for the -l case
> only.
Ok, sounds like most people agree on -l being readable...
> > > > odd use of useracc(), printf(), etc, in the ioctl code. I suspect this
> > >
> > > well, do you mean odd use of printf as in providing diagnostics to catch
> > > mismatched userland/kernel?
> >
> > Spamming the console with messages can have debilitating effects on the
> > operation of the system if performed by unprivileged users... I.e., if
> > you're using a serial console.
>
> I.e., if you are using a console. Spamming of syscons consoles messes them
> up too, and is more likely to cause panics since the syscons console output
> is less reentrant than sio console output.
I removed those printf's since they really didn't contain any information
more than the user already knew..
> > > for useracc, it checks to make sure that various pointers passed to it
> > > are either readable or writable. I don't see this as odd. Or is there
> > > another better method of checking user data when accessing user space
> > > buffers?
> >
> > Generally, calls to useracc() are redundant with the existing checks in
> > our copyin/out routines, or are signs that the proper routines aren't
> > being used.
>
> useracc() can be used to "improve" error handling. That is done here.
> The "improvements" include dangerous printf()s and returning the
> undocumented errno EACCES instead of the incompletely documented one EFAULT.
Doh, reminder, update documentation.. I'm going to let the copyout
error be returned.
> > All of the fuword/suword/copyin/copyout/uio routines already
> > perform any necessary checks; manual checking is race-prone in a
> > multi-threaded smp environment regardless.
>
> Please check the error handling carefully if you remove the useracc()'s.
> The one for writing the results back to userland gets replaced by checks
> in copyout() for each part of the results. I think we give up after the
> first copyout() error and return that error. That seems right.
actually, we had a problem where if we tried to copy out, but failed,
we would still increment num_matches even though the last one was bogus..
> > The (cio==NULL) test is also
> > redundant.
>
> It is just bogus. cio is a kernel pointer and we need it to be more than
> non-NULL to access it. sys_generic.c:ioctl() guarantees this provided
> the definition of PCICGETCONF is correct.
>
> > 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.
--
John-Mark Gurney Voice: +1 415 225 5579
"All that I will do, has been done, All that I have, has not."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030617052917.GF73854>