Re: PATCH: Fix PCI Config Space union size on VMM
On Mon, Sep 07, 2020 at 06:03:00PM -0500, Jordan Hargrave wrote:
> This code fixes the pci device union for accessing PCI config space >= 0x40
>
> Running pcidump -xxx in a virtual machine would return garbage data due to
> union overlap
>
Thanks, looks good from my perspective.
-ml
> On Mon, Sep 07, 2020 at 05:52:55PM -0500, Jordan Hargrave wrote:
> > Index: pci.h
> > ===
> > RCS file: /cvs/src/usr.sbin/vmd/pci.h,v
> > retrieving revision 1.7
> > diff -u -p -u -r1.7 pci.h
> > --- pci.h 17 Sep 2017 23:07:56 - 1.7
> > +++ pci.h 7 Sep 2020 22:48:09 -
> > @@ -32,43 +32,44 @@ typedef int (*pci_iobar_fn_t)(int dir, u
> > void *, uint8_t);
> > typedef int (*pci_mmiobar_fn_t)(int dir, uint32_t ofs, uint32_t *data);
> >
> > -union pci_dev {
> > - uint32_t pd_cfg_space[PCI_CONFIG_SPACE_SIZE / 4];
> >
> > - struct {
> > - uint16_t pd_vid;
> > - uint16_t pd_did;
> > - uint16_t pd_cmd;
> > - uint16_t pd_status;
> > - uint8_t pd_rev;
> > - uint8_t pd_prog_if;
> > - uint8_t pd_subclass;
> > - uint8_t pd_class;
> > - uint8_t pd_cache_size;
> > - uint8_t pd_lat_timer;
> > - uint8_t pd_header_type;
> > - uint8_t pd_bist;
> > - uint32_t pd_bar[PCI_MAX_BARS];
> > - uint32_t pd_cardbus_cis;
> > - uint16_t pd_subsys_vid;
> > - uint16_t pd_subsys_id;
> > - uint32_t pd_exp_rom_addr;
> > - uint8_t pd_cap;
> > - uint32_t pd_reserved0 : 24;
> > - uint32_t pd_reserved1;
> > - uint8_t pd_irq;
> > - uint8_t pd_int;
> > - uint8_t pd_min_grant;
> > - uint8_t pd_max_grant;
> > +struct pci_dev {
> > + union {
> > + uint32_t pd_cfg_space[PCI_CONFIG_SPACE_SIZE / 4];
> > + struct {
> > + uint16_t pd_vid;
> > + uint16_t pd_did;
> > + uint16_t pd_cmd;
> > + uint16_t pd_status;
> > + uint8_t pd_rev;
> > + uint8_t pd_prog_if;
> > + uint8_t pd_subclass;
> > + uint8_t pd_class;
> > + uint8_t pd_cache_size;
> > + uint8_t pd_lat_timer;
> > + uint8_t pd_header_type;
> > + uint8_t pd_bist;
> > + uint32_t pd_bar[PCI_MAX_BARS];
> > + uint32_t pd_cardbus_cis;
> > + uint16_t pd_subsys_vid;
> > + uint16_t pd_subsys_id;
> > + uint32_t pd_exp_rom_addr;
> > + uint8_t pd_cap;
> > + uint32_t pd_reserved0 : 24;
> > + uint32_t pd_reserved1;
> > + uint8_t pd_irq;
> > + uint8_t pd_int;
> > + uint8_t pd_min_grant;
> > + uint8_t pd_max_grant;
> > + } __packed;
> > + };
> > + uint8_t pd_bar_ct;
> > + pci_cs_fn_t pd_csfunc;
> >
> > - uint8_t pd_bar_ct;
> > - pci_cs_fn_t pd_csfunc;
> > -
> > - uint8_t pd_bartype[PCI_MAX_BARS];
> > - uint32_t pd_barsize[PCI_MAX_BARS];
> > - void *pd_barfunc[PCI_MAX_BARS];
> > - void *pd_bar_cookie[PCI_MAX_BARS];
> > - } __packed;
> > + uint8_t pd_bartype[PCI_MAX_BARS];
> > + uint32_t pd_barsize[PCI_MAX_BARS];
> > + void *pd_barfunc[PCI_MAX_BARS];
> > + void *pd_bar_cookie[PCI_MAX_BARS];
> > };
> >
> > struct pci {
> > @@ -79,7 +80,7 @@ struct pci {
> > uint32_t pci_addr_reg;
> > uint32_t pci_data_reg;
> >
> > - union pci_dev pci_devices[PCI_CONFIG_MAX_DEV];
> > + struct pci_dev pci_devices[PCI_CONFIG_MAX_DEV];
> > };
> >
> > void pci_handle_address_reg(struct vm_run_params *);
> >
>
Re: issignal() w/o KERNEL_LOCK()
On Wed, Sep 09, 2020 at 06:23:22PM +0200, Martin Pieuchot wrote: > On 09/09/20(Wed) 10:02, Claudio Jeker wrote: > > On Wed, Sep 09, 2020 at 08:33:30AM +0200, Martin Pieuchot wrote: > > > Per-process data structures needed to suspend the execution of threads > > > are since recently protected by the SCHED_LOCK(). So the KERNEL_LOCK() > > > dance inside issignal() is no longer necessary and can be removed, ok? > > > > This is not quite right. single_thread_set() still needs the > > KERNEL_LOCK() to avoid racing against itself. > > Which data structure still requires the KERNEL_LOCK()? Are you talking > about the per-process list of threads? > > Isn't the SCHED_LOCK() enough to prevent racing against itself? The main problem is a race for setting ps_single. It is possible that two threads enter single_thread_set() both succeed on single_thread_check() but now the first thread will get stomped over by the second thread. The code is not quite right and it needs to be restructured to be safe. It probably needs a similar setup finish dance like sleep. -- :wq Claudio
Re: issignal() w/o KERNEL_LOCK()
On 09/09/20(Wed) 10:02, Claudio Jeker wrote: > On Wed, Sep 09, 2020 at 08:33:30AM +0200, Martin Pieuchot wrote: > > Per-process data structures needed to suspend the execution of threads > > are since recently protected by the SCHED_LOCK(). So the KERNEL_LOCK() > > dance inside issignal() is no longer necessary and can be removed, ok? > > This is not quite right. single_thread_set() still needs the > KERNEL_LOCK() to avoid racing against itself. Which data structure still requires the KERNEL_LOCK()? Are you talking about the per-process list of threads? Isn't the SCHED_LOCK() enough to prevent racing against itself?
Re: sigismasked()
On 2020-09-09 09:35, Martin Pieuchot wrote:
Simple helper function to centralize the manipulation of `ps_sigignore'
and `p_sigmask' in kern/kern_sig.c and later on add the corresponding
asserts, ok?
Yes please! OK pirofti@
Index: kern/kern_sig.c
===
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.260
diff -u -p -r1.260 kern_sig.c
--- kern/kern_sig.c 26 Aug 2020 03:16:53 - 1.260
+++ kern/kern_sig.c 8 Sep 2020 05:46:25 -
@@ -1486,6 +1486,22 @@ sigexit(struct proc *p, int signum)
/* NOTREACHED */
}
+/*
+ * Return 1 if `sig', a given signal, is ignored or masked for `p', a given
+ * thread, and 0 otherwise.
+ */
+int
+sigismasked(struct proc *p, int sig)
+{
+ struct process *pr = p->p_p;
+
+ if ((pr->ps_sigacts->ps_sigignore & sigmask(sig)) ||
+ (p->p_sigmask & sigmask(sig)))
+ return 1;
+
+ return 0;
+}
+
int nosuidcoredump = 1;
struct coredump_iostate {
Index: kern/tty_pty.c
===
RCS file: /cvs/src/sys/kern/tty_pty.c,v
retrieving revision 1.103
diff -u -p -r1.103 tty_pty.c
--- kern/tty_pty.c 20 Jul 2020 14:34:16 - 1.103
+++ kern/tty_pty.c 8 Sep 2020 05:28:46 -
@@ -289,8 +289,7 @@ ptsread(dev_t dev, struct uio *uio, int
again:
if (pti->pt_flags & PF_REMOTE) {
while (isbackground(pr, tp)) {
- if ((pr->ps_sigacts->ps_sigignore & sigmask(SIGTTIN)) ||
- (p->p_sigmask & sigmask(SIGTTIN)) ||
+ if (sigismasked(p, SIGTTIN) ||
pr->ps_pgrp->pg_jobc == 0 ||
pr->ps_flags & PS_PPWAIT)
return (EIO);
Index: kern/tty.c
===
RCS file: /cvs/src/sys/kern/tty.c,v
retrieving revision 1.163
diff -u -p -r1.163 tty.c
--- kern/tty.c 22 Jul 2020 17:39:50 - 1.163
+++ kern/tty.c 8 Sep 2020 05:28:46 -
@@ -744,8 +744,7 @@ ttioctl(struct tty *tp, u_long cmd, cadd
case TIOCSWINSZ:
while (isbackground(pr, tp) &&
(pr->ps_flags & PS_PPWAIT) == 0 &&
- (pr->ps_sigacts->ps_sigignore & sigmask(SIGTTOU)) == 0 &&
- (p->p_sigmask & sigmask(SIGTTOU)) == 0) {
+ !sigismasked(p, SIGTTOU)) {
if (pr->ps_pgrp->pg_jobc == 0)
return (EIO);
pgsignal(pr->ps_pgrp, SIGTTOU, 1);
@@ -1498,8 +1497,7 @@ loop: lflag = tp->t_lflag;
* Hang process if it's in the background.
*/
if (isbackground(pr, tp)) {
- if ((pr->ps_sigacts->ps_sigignore & sigmask(SIGTTIN)) ||
- (p->p_sigmask & sigmask(SIGTTIN)) ||
+ if (sigismasked(p, SIGTTIN) ||
pr->ps_flags & PS_PPWAIT || pr->ps_pgrp->pg_jobc == 0) {
error = EIO;
goto out;
@@ -1749,8 +1747,7 @@ loop:
pr = p->p_p;
if (isbackground(pr, tp) &&
ISSET(tp->t_lflag, TOSTOP) && (pr->ps_flags & PS_PPWAIT) == 0 &&
- (pr->ps_sigacts->ps_sigignore & sigmask(SIGTTOU)) == 0 &&
- (p->p_sigmask & sigmask(SIGTTOU)) == 0) {
+ !sigismasked(p, SIGTTOU)) {
if (pr->ps_pgrp->pg_jobc == 0) {
error = EIO;
goto out;
Index: sys/signalvar.h
===
RCS file: /cvs/src/sys/sys/signalvar.h,v
retrieving revision 1.41
diff -u -p -r1.41 signalvar.h
--- sys/signalvar.h 10 May 2020 00:56:06 - 1.41
+++ sys/signalvar.h 8 Sep 2020 05:29:10 -
@@ -126,6 +126,7 @@ voidsiginit(struct process *);
void trapsignal(struct proc *p, int sig, u_long code, int type,
union sigval val);
void sigexit(struct proc *, int);
+intsigismasked(struct proc *, int);
int sigonstack(size_t);
void setsigvec(struct proc *, int, struct sigaction *);
int killpg1(struct proc *, int, int, int);
Re: [WIP FAQ13] New section for webcam usage
On Tue, Sep 08, 2020 at 07:13:10PM +0200, Stefan Hagen wrote: Hello Stefan, > An audio device is special in a way that it has playback and recording > capabilities in one device. The sysctl is used to allow playback (by > default) but not allow recording. > > Video (as in webcam) is always a recording device, which shouldn't be > allowed to access in a default install (in contrast to audio playback). Personally I only set kern.audio.record to 1 immediately before I want to record from my microphone: I turn it back to 0 immediately afterwards, as I don't want a program to record audio when I'm not expecting it to. It would be impractical to do this if it was at a group level, as I would have to log out and back in to make the equivalent change. [I approximate this change by chown'ing a-rwx /dev/video after I've used my webcam though, of course, any other program can chown it back afterwards, so this is gives only very limited security.] Laurie
Re: issignal() w/o KERNEL_LOCK()
On Wed, Sep 09, 2020 at 08:33:30AM +0200, Martin Pieuchot wrote:
> Per-process data structures needed to suspend the execution of threads
> are since recently protected by the SCHED_LOCK(). So the KERNEL_LOCK()
> dance inside issignal() is no longer necessary and can be removed, ok?
This is not quite right. single_thread_set() still needs the
KERNEL_LOCK() to avoid racing against itself.
> Note that CURSIG() is currently always called with the KERNEL_LOCK()
> held so the code below is redundant.
This is a very weak argument. The goal is to call CURSIG() without the
KERNEL_LOCK() but it is currently obvious that single_thread_set() needs
the KERNEL_LOCK() and needs to be fixed first. Fixing single_thread_set
and single_thread_clear (which I think is actually fine) is still necessary.
While the diff is OK it is mainly so because of side-effects that will be
removed in the future.
> This is a step towards getting signal handling out of ze big lock.
>
> Index: kern/kern_sig.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.260
> diff -u -p -r1.260 kern_sig.c
> --- kern/kern_sig.c 26 Aug 2020 03:16:53 - 1.260
> +++ kern/kern_sig.c 8 Sep 2020 05:48:51 -
> @@ -1203,11 +1203,7 @@ issignal(struct proc *p)
> signum != SIGKILL) {
> pr->ps_xsig = signum;
>
> - if (dolock)
> - KERNEL_LOCK();
> single_thread_set(p, SINGLE_PTRACE, 0);
> - if (dolock)
> - KERNEL_UNLOCK();
>
> if (dolock)
> SCHED_LOCK(s);
> @@ -1215,11 +1211,7 @@ issignal(struct proc *p)
> if (dolock)
> SCHED_UNLOCK(s);
>
> - if (dolock)
> - KERNEL_LOCK();
> single_thread_clear(p, 0);
> - if (dolock)
> - KERNEL_UNLOCK();
>
> /*
>* If we are no longer being traced, or the parent
> @@ -1484,6 +1476,22 @@ sigexit(struct proc *p, int signum)
> }
> exit1(p, 0, signum, EXIT_NORMAL);
> /* NOTREACHED */
> +}
> +
> +/*
> + * Return 1 if `sig', a given signal, is ignored or masked for `p', a given
> + * thread, and 0 otherwise.
> + */
> +int
> +sigismasked(struct proc *p, int sig)
> +{
> + struct process *pr = p->p_p;
> +
> + if ((pr->ps_sigacts->ps_sigignore & sigmask(sig)) ||
> + (p->p_sigmask & sigmask(sig)))
> + return 1;
> +
> + return 0;
> }
>
> int nosuidcoredump = 1;
>
--
:wq Claudio
KASSERT() for VOP_*
This is mostly the same diff that has been backed out months ago with the VOP_CLOSE() case fixed. VOP_CLOSE() can accept a NULL argument instead of `curproc' when garbage collecting passed FDs. The intent is to stop passing a "struct proc *" when a function applies only to `curproc'. Synchronization/locking primitives are obviously different if a CPU can modify the fields of any thread or only of the current one. Index: kern/vfs_vops.c === RCS file: /cvs/src/sys/kern/vfs_vops.c,v retrieving revision 1.28 diff -u -p -r1.28 vfs_vops.c --- kern/vfs_vops.c 8 Apr 2020 08:07:51 - 1.28 +++ kern/vfs_vops.c 27 Apr 2020 08:10:02 - @@ -145,6 +145,8 @@ VOP_OPEN(struct vnode *vp, int mode, str a.a_cred = cred; a.a_p = p; + KASSERT(p == curproc); + if (vp->v_op->vop_open == NULL) return (EOPNOTSUPP); @@ -164,6 +166,7 @@ VOP_CLOSE(struct vnode *vp, int fflag, s a.a_cred = cred; a.a_p = p; + KASSERT(p == NULL || p == curproc); ASSERT_VP_ISLOCKED(vp); if (vp->v_op->vop_close == NULL) @@ -184,6 +187,7 @@ VOP_ACCESS(struct vnode *vp, int mode, s a.a_cred = cred; a.a_p = p; + KASSERT(p == curproc); ASSERT_VP_ISLOCKED(vp); if (vp->v_op->vop_access == NULL) @@ -202,6 +206,7 @@ VOP_GETATTR(struct vnode *vp, struct vat a.a_cred = cred; a.a_p = p; + KASSERT(p == curproc); if (vp->v_op->vop_getattr == NULL) return (EOPNOTSUPP); @@ -219,6 +224,7 @@ VOP_SETATTR(struct vnode *vp, struct vat a.a_cred = cred; a.a_p = p; + KASSERT(p == curproc); ASSERT_VP_ISLOCKED(vp); if (vp->v_op->vop_setattr == NULL) @@ -282,6 +288,7 @@ VOP_IOCTL(struct vnode *vp, u_long comma a.a_cred = cred; a.a_p = p; + KASSERT(p == curproc); if (vp->v_op->vop_ioctl == NULL) return (EOPNOTSUPP); @@ -300,6 +307,7 @@ VOP_POLL(struct vnode *vp, int fflag, in a.a_events = events; a.a_p = p; + KASSERT(p == curproc); if (vp->v_op->vop_poll == NULL) return (EOPNOTSUPP); @@ -344,6 +352,7 @@ VOP_FSYNC(struct vnode *vp, struct ucred a.a_waitfor = waitfor; a.a_p = p; + KASSERT(p == curproc); ASSERT_VP_ISLOCKED(vp); if (vp->v_op->vop_fsync == NULL) @@ -565,6 +574,7 @@ VOP_INACTIVE(struct vnode *vp, struct pr a.a_vp = vp; a.a_p = p; + KASSERT(p == curproc); ASSERT_VP_ISLOCKED(vp); if (vp->v_op->vop_inactive == NULL) @@ -581,6 +591,7 @@ VOP_RECLAIM(struct vnode *vp, struct pro a.a_vp = vp; a.a_p = p; + KASSERT(p == curproc); if (vp->v_op->vop_reclaim == NULL) return (EOPNOTSUPP);
sigismasked()
Simple helper function to centralize the manipulation of `ps_sigignore'
and `p_sigmask' in kern/kern_sig.c and later on add the corresponding
asserts, ok?
Index: kern/kern_sig.c
===
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.260
diff -u -p -r1.260 kern_sig.c
--- kern/kern_sig.c 26 Aug 2020 03:16:53 - 1.260
+++ kern/kern_sig.c 8 Sep 2020 05:46:25 -
@@ -1486,6 +1486,22 @@ sigexit(struct proc *p, int signum)
/* NOTREACHED */
}
+/*
+ * Return 1 if `sig', a given signal, is ignored or masked for `p', a given
+ * thread, and 0 otherwise.
+ */
+int
+sigismasked(struct proc *p, int sig)
+{
+ struct process *pr = p->p_p;
+
+ if ((pr->ps_sigacts->ps_sigignore & sigmask(sig)) ||
+ (p->p_sigmask & sigmask(sig)))
+ return 1;
+
+ return 0;
+}
+
int nosuidcoredump = 1;
struct coredump_iostate {
Index: kern/tty_pty.c
===
RCS file: /cvs/src/sys/kern/tty_pty.c,v
retrieving revision 1.103
diff -u -p -r1.103 tty_pty.c
--- kern/tty_pty.c 20 Jul 2020 14:34:16 - 1.103
+++ kern/tty_pty.c 8 Sep 2020 05:28:46 -
@@ -289,8 +289,7 @@ ptsread(dev_t dev, struct uio *uio, int
again:
if (pti->pt_flags & PF_REMOTE) {
while (isbackground(pr, tp)) {
- if ((pr->ps_sigacts->ps_sigignore & sigmask(SIGTTIN)) ||
- (p->p_sigmask & sigmask(SIGTTIN)) ||
+ if (sigismasked(p, SIGTTIN) ||
pr->ps_pgrp->pg_jobc == 0 ||
pr->ps_flags & PS_PPWAIT)
return (EIO);
Index: kern/tty.c
===
RCS file: /cvs/src/sys/kern/tty.c,v
retrieving revision 1.163
diff -u -p -r1.163 tty.c
--- kern/tty.c 22 Jul 2020 17:39:50 - 1.163
+++ kern/tty.c 8 Sep 2020 05:28:46 -
@@ -744,8 +744,7 @@ ttioctl(struct tty *tp, u_long cmd, cadd
case TIOCSWINSZ:
while (isbackground(pr, tp) &&
(pr->ps_flags & PS_PPWAIT) == 0 &&
- (pr->ps_sigacts->ps_sigignore & sigmask(SIGTTOU)) == 0 &&
- (p->p_sigmask & sigmask(SIGTTOU)) == 0) {
+ !sigismasked(p, SIGTTOU)) {
if (pr->ps_pgrp->pg_jobc == 0)
return (EIO);
pgsignal(pr->ps_pgrp, SIGTTOU, 1);
@@ -1498,8 +1497,7 @@ loop: lflag = tp->t_lflag;
* Hang process if it's in the background.
*/
if (isbackground(pr, tp)) {
- if ((pr->ps_sigacts->ps_sigignore & sigmask(SIGTTIN)) ||
- (p->p_sigmask & sigmask(SIGTTIN)) ||
+ if (sigismasked(p, SIGTTIN) ||
pr->ps_flags & PS_PPWAIT || pr->ps_pgrp->pg_jobc == 0) {
error = EIO;
goto out;
@@ -1749,8 +1747,7 @@ loop:
pr = p->p_p;
if (isbackground(pr, tp) &&
ISSET(tp->t_lflag, TOSTOP) && (pr->ps_flags & PS_PPWAIT) == 0 &&
- (pr->ps_sigacts->ps_sigignore & sigmask(SIGTTOU)) == 0 &&
- (p->p_sigmask & sigmask(SIGTTOU)) == 0) {
+ !sigismasked(p, SIGTTOU)) {
if (pr->ps_pgrp->pg_jobc == 0) {
error = EIO;
goto out;
Index: sys/signalvar.h
===
RCS file: /cvs/src/sys/sys/signalvar.h,v
retrieving revision 1.41
diff -u -p -r1.41 signalvar.h
--- sys/signalvar.h 10 May 2020 00:56:06 - 1.41
+++ sys/signalvar.h 8 Sep 2020 05:29:10 -
@@ -126,6 +126,7 @@ voidsiginit(struct process *);
void trapsignal(struct proc *p, int sig, u_long code, int type,
union sigval val);
void sigexit(struct proc *, int);
+intsigismasked(struct proc *, int);
intsigonstack(size_t);
void setsigvec(struct proc *, int, struct sigaction *);
intkillpg1(struct proc *, int, int, int);
issignal() w/o KERNEL_LOCK()
Per-process data structures needed to suspend the execution of threads
are since recently protected by the SCHED_LOCK(). So the KERNEL_LOCK()
dance inside issignal() is no longer necessary and can be removed, ok?
Note that CURSIG() is currently always called with the KERNEL_LOCK()
held so the code below is redundant.
This is a step towards getting signal handling out of ze big lock.
Index: kern/kern_sig.c
===
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.260
diff -u -p -r1.260 kern_sig.c
--- kern/kern_sig.c 26 Aug 2020 03:16:53 - 1.260
+++ kern/kern_sig.c 8 Sep 2020 05:48:51 -
@@ -1203,11 +1203,7 @@ issignal(struct proc *p)
signum != SIGKILL) {
pr->ps_xsig = signum;
- if (dolock)
- KERNEL_LOCK();
single_thread_set(p, SINGLE_PTRACE, 0);
- if (dolock)
- KERNEL_UNLOCK();
if (dolock)
SCHED_LOCK(s);
@@ -1215,11 +1211,7 @@ issignal(struct proc *p)
if (dolock)
SCHED_UNLOCK(s);
- if (dolock)
- KERNEL_LOCK();
single_thread_clear(p, 0);
- if (dolock)
- KERNEL_UNLOCK();
/*
* If we are no longer being traced, or the parent
@@ -1484,6 +1476,22 @@ sigexit(struct proc *p, int signum)
}
exit1(p, 0, signum, EXIT_NORMAL);
/* NOTREACHED */
+}
+
+/*
+ * Return 1 if `sig', a given signal, is ignored or masked for `p', a given
+ * thread, and 0 otherwise.
+ */
+int
+sigismasked(struct proc *p, int sig)
+{
+ struct process *pr = p->p_p;
+
+ if ((pr->ps_sigacts->ps_sigignore & sigmask(sig)) ||
+ (p->p_sigmask & sigmask(sig)))
+ return 1;
+
+ return 0;
}
int nosuidcoredump = 1;
