Paths

Table of Contentst

-
usr.bin/cmp/
-
cmp/
3/8
cmp.c

Differential D18860

Fix cmp(1) to handle the case where standard streams are closed.
ClosedPublic
Actions

Authored by markj on Jan 16 2019, 8:39 PM.

Details

Reviewers

oshogbo

Group Reviewers

capsicum

Commits

rS343117: Fix handling of rights on stdio streams.

Summary

Use caph_limit_stdio() before opening input files. This function
suppresses EBADF. When further limiting stdin, make sure STDIN_FILENO
isn't being used for one of the input files.

Fix some style bugs while here.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 21987
Build 21221: arc lint + arc unit

Event Timeline

markj created this revision.Jan 16 2019, 8:39 PM

Harbormaster completed remote builds in B21986: Diff 52898.Jan 16 2019, 8:39 PM

markj added reviewers: oshogbo, capsicum.Jan 16 2019, 8:40 PM

oshogbo added inline comments.Jan 16 2019, 9:26 PM

usr.bin/cmp/cmp.c
119	stderr?
119	I miss read sorry.

oshogbo added inline comments.Jan 16 2019, 9:39 PM

usr.bin/cmp/cmp.c
167	Hym not sure if I get this. special is set if fd1 or fd2 is set to the STDIN right? So we don't need to check those.
169	I'm not sure if I'm understand that correctly. If we have a problem with closed STDIN dosent this return EBADF as well? The special will not be set but STDIN will be closed, so this should fail I think.

markj added inline comments.Jan 16 2019, 9:45 PM

usr.bin/cmp/cmp.c
167	Yes, but if !special and the shell doesn't provide stdin, then STDIN_FILENO will be used for one of the input files.
169	If stdin is closed, then file descriptor 0 is available and will be claimed by fd1 or fd2. You are right though, this is making the assumption that open() will return the smallest available descriptor.

Verify that STDIN_FILENO is in use before limiting rights on it. In practice, it will always be in use: if STDIN_FILENO is not provided by the shell, then it will be claimed for fd1. But, we should not make assumptions about the kernel's fd allocation policy.

Harbormaster completed remote builds in B21987: Diff 52900.Jan 16 2019, 9:57 PM

oshogbo added inline comments.Jan 17 2019, 11:08 AM

usr.bin/cmp/cmp.c
170	Let's just remove this whole block. If the stdin is used lets limit it using caph_limit_stdio. If it is but not used do we care to limit it further? In my opinion this is a minimal value if not even none and complicate code a lot.

markj added inline comments.Jan 17 2019, 4:00 PM

usr.bin/cmp/cmp.c
170	Fair enough. I agree that it doesn't accomplish much: we're just removing read caps from the descriptor.

Don't bother limiting rights on stdin beyond what caph_limit_stdio()
does.

Harbormaster completed remote builds in B22002: Diff 52928.Jan 17 2019, 4:02 PM

oshogbo accepted this revision.Jan 17 2019, 5:09 PM

This revision is now accepted and ready to land.Jan 17 2019, 5:09 PM

Closed by commit rS343117: Fix handling of rights on stdio streams. (authored by markj). · Explain WhyJan 17 2019, 5:36 PM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: imp. · View Herald TranscriptJan 17 2019, 5:36 PM

Revision Contents
Changeset List

Path

Size

usr.bin/

cmp/

cmp.c

31 lines

Diff 52900

View Options

usr.bin/cmp/cmp.c

Show First 20 Lines • Show All 164 Lines • ▼ Show 20 Lines
struct pci_xhci_dev_ep {		struct pci_xhci_dev_ep {
union {		union {
struct xhci_trb *_epu_tr;		struct xhci_trb *_epu_tr;
struct xhci_stream_ctx *_epu_sctx;		struct xhci_stream_ctx *_epu_sctx;
} _ep_trbsctx;		} _ep_trbsctx;
#define ep_tr _ep_trbsctx._epu_tr		#define ep_tr _ep_trbsctx._epu_tr
#define ep_sctx _ep_trbsctx._epu_sctx		#define ep_sctx _ep_trbsctx._epu_sctx

		/*
		markjUnsubmitted Not Done Inline Actions It'd be helpful to have a comment indicating that this is a shadow of the max_pstreams field of epctx0. markj: It'd be helpful to have a comment indicating that this is a shadow of the max_pstreams field of…
		markjUnsubmitted Not Done Inline Actions (Also note that ep_maxpstreams might be a more accurate name.) markj: (Also note that ep_maxpstreams might be a more accurate name.)
		jhbAuthorUnsubmitted Done Inline Actions I will do both of those. Much of this file is under-commented though and would seem to assume knowledge of the XHCI spec in which case a more properly named field (I agree with ep_maxpstreams or possibly even ep_MaxPStreams to match the spec) is perhaps sufficient? The comment would likely say something like "Cached value of the MaxPStreams field from the endpoint context." jhb: I will do both of those. Much of this file is under-commented though and would seem to assume…
		* Caches the value of MaxPStreams from the endpoint context
		* when an endpoint is initialized and is used to validate the
		* use of ep_ringaddr vs ep_sctx_trbs[] as well as the length
		* of ep_sctx_trbs[].
		*/
		uint32_t ep_MaxPStreams;
union {		union {
struct pci_xhci_trb_ring _epu_trb;		struct pci_xhci_trb_ring _epu_trb;
struct pci_xhci_trb_ring *_epu_sctx_trbs;		struct pci_xhci_trb_ring *_epu_sctx_trbs;
} _ep_trb_rings;		} _ep_trb_rings;
#define ep_ringaddr _ep_trb_rings._epu_trb.ringaddr		#define ep_ringaddr _ep_trb_rings._epu_trb.ringaddr
#define ep_ccs _ep_trb_rings._epu_trb.ccs		#define ep_ccs _ep_trb_rings._epu_trb.ccs
#define ep_sctx_trbs _ep_trb_rings._epu_sctx_trbs		#define ep_sctx_trbs _ep_trb_rings._epu_sctx_trbs

▲ Show 20 Lines • Show All 487 Lines • ▼ Show 20 Lines	pci_xhci_init_ep(struct pci_xhci_dev_emu *dev, int epid)
} else {		} else {
DPRINTF(("init_ep %d with no pstreams", epid));		DPRINTF(("init_ep %d with no pstreams", epid));
devep->ep_ringaddr = ep_ctx->qwEpCtx2 &		devep->ep_ringaddr = ep_ctx->qwEpCtx2 &
XHCI_EPCTX_2_TR_DQ_PTR_MASK;		XHCI_EPCTX_2_TR_DQ_PTR_MASK;
devep->ep_ccs = XHCI_EPCTX_2_DCS_GET(ep_ctx->qwEpCtx2);		devep->ep_ccs = XHCI_EPCTX_2_DCS_GET(ep_ctx->qwEpCtx2);
devep->ep_tr = XHCI_GADDR(dev->xsc, devep->ep_ringaddr);		devep->ep_tr = XHCI_GADDR(dev->xsc, devep->ep_ringaddr);
DPRINTF(("init_ep tr DCS %x", devep->ep_ccs));		DPRINTF(("init_ep tr DCS %x", devep->ep_ccs));
}		}
		devep->ep_MaxPStreams = pstreams;

if (devep->ep_xfer == NULL) {		if (devep->ep_xfer == NULL) {
devep->ep_xfer = malloc(sizeof(struct usb_data_xfer));		devep->ep_xfer = malloc(sizeof(struct usb_data_xfer));
USB_DATA_XFER_INIT(devep->ep_xfer);		USB_DATA_XFER_INIT(devep->ep_xfer);
}		}
}		}

static void		static void
pci_xhci_disable_ep(struct pci_xhci_dev_emu *dev, int epid)		pci_xhci_disable_ep(struct pci_xhci_dev_emu *dev, int epid)
{		{
struct xhci_dev_ctx *dev_ctx;		struct xhci_dev_ctx *dev_ctx;
struct pci_xhci_dev_ep *devep;		struct pci_xhci_dev_ep *devep;
struct xhci_endp_ctx *ep_ctx;		struct xhci_endp_ctx *ep_ctx;

DPRINTF(("pci_xhci disable_ep %d", epid));		DPRINTF(("pci_xhci disable_ep %d", epid));

dev_ctx = dev->dev_ctx;		dev_ctx = dev->dev_ctx;
ep_ctx = &dev_ctx->ctx_ep[epid];		ep_ctx = &dev_ctx->ctx_ep[epid];
ep_ctx->dwEpCtx0 = (ep_ctx->dwEpCtx0 & ~0x7) \| XHCI_ST_EPCTX_DISABLED;		ep_ctx->dwEpCtx0 = (ep_ctx->dwEpCtx0 & ~0x7) \| XHCI_ST_EPCTX_DISABLED;

devep = &dev->eps[epid];		devep = &dev->eps[epid];
if (XHCI_EPCTX_0_MAXP_STREAMS_GET(ep_ctx->dwEpCtx0) > 0 &&		if (devep->ep_MaxPStreams > 0)
devep->ep_sctx_trbs != NULL)
free(devep->ep_sctx_trbs);		free(devep->ep_sctx_trbs);

if (devep->ep_xfer != NULL) {		if (devep->ep_xfer != NULL) {
free(devep->ep_xfer);		free(devep->ep_xfer);
devep->ep_xfer = NULL;		devep->ep_xfer = NULL;
}		}

memset(devep, 0, sizeof(struct pci_xhci_dev_ep));		memset(devep, 0, sizeof(struct pci_xhci_dev_ep));
}		}
▲ Show 20 Lines • Show All 443 Lines • ▼ Show 20 Lines	pci_xhci_cmd_reset_ep(struct pci_xhci_softc *sc, uint32_t slot,

dev_ctx = dev->dev_ctx;		dev_ctx = dev->dev_ctx;
assert(dev_ctx != NULL);		assert(dev_ctx != NULL);

ep_ctx = &dev_ctx->ctx_ep[epid];		ep_ctx = &dev_ctx->ctx_ep[epid];

ep_ctx->dwEpCtx0 = (ep_ctx->dwEpCtx0 & ~0x7) \| XHCI_ST_EPCTX_STOPPED;		ep_ctx->dwEpCtx0 = (ep_ctx->dwEpCtx0 & ~0x7) \| XHCI_ST_EPCTX_STOPPED;

if (XHCI_EPCTX_0_MAXP_STREAMS_GET(ep_ctx->dwEpCtx0) == 0)		if (devep->ep_MaxPStreams == 0)
ep_ctx->qwEpCtx2 = devep->ep_ringaddr \| devep->ep_ccs;		ep_ctx->qwEpCtx2 = devep->ep_ringaddr \| devep->ep_ccs;

DPRINTF(("pci_xhci: reset ep[%u] %08x %08x %016lx %08x",		DPRINTF(("pci_xhci: reset ep[%u] %08x %08x %016lx %08x",
epid, ep_ctx->dwEpCtx0, ep_ctx->dwEpCtx1, ep_ctx->qwEpCtx2,		epid, ep_ctx->dwEpCtx0, ep_ctx->dwEpCtx1, ep_ctx->qwEpCtx2,
ep_ctx->dwEpCtx4));		ep_ctx->dwEpCtx4));

if (type == XHCI_TRB_TYPE_RESET_EP &&		if (type == XHCI_TRB_TYPE_RESET_EP &&
(dev->dev_ue->ue_reset == NULL \|\|		(dev->dev_ue->ue_reset == NULL \|\|
dev->dev_ue->ue_reset(dev->dev_sc) < 0)) {		dev->dev_ue->ue_reset(dev->dev_sc) < 0)) {
cmderr = XHCI_TRB_ERROR_ENDP_NOT_ON;		cmderr = XHCI_TRB_ERROR_ENDP_NOT_ON;
goto done;		goto done;
}		}

done:		done:
return (cmderr);		return (cmderr);
}		}


static uint32_t		static uint32_t
pci_xhci_find_stream(struct pci_xhci_softc sc, struct xhci_endp_ctx ep,		pci_xhci_find_stream(struct pci_xhci_softc sc, struct xhci_endp_ctx ep,
uint32_t streamid, struct xhci_stream_ctx **osctx)		struct pci_xhci_dev_ep *devep, uint32_t streamid,
		struct xhci_stream_ctx **osctx)
{		{
struct xhci_stream_ctx *sctx;		struct xhci_stream_ctx *sctx;
uint32_t maxpstreams;

maxpstreams = XHCI_EPCTX_0_MAXP_STREAMS_GET(ep->dwEpCtx0);		if (devep->ep_MaxPStreams == 0)
if (maxpstreams == 0)
return (XHCI_TRB_ERROR_TRB);		return (XHCI_TRB_ERROR_TRB);

if (maxpstreams > XHCI_STREAMS_MAX)		if (devep->ep_MaxPStreams > XHCI_STREAMS_MAX)
return (XHCI_TRB_ERROR_INVALID_SID);		return (XHCI_TRB_ERROR_INVALID_SID);

if (XHCI_EPCTX_0_LSA_GET(ep->dwEpCtx0) == 0) {		if (XHCI_EPCTX_0_LSA_GET(ep->dwEpCtx0) == 0) {
DPRINTF(("pci_xhci: find_stream; LSA bit not set"));		DPRINTF(("pci_xhci: find_stream; LSA bit not set"));
return (XHCI_TRB_ERROR_INVALID_SID);		return (XHCI_TRB_ERROR_INVALID_SID);
}		}

/* only support primary stream */		/* only support primary stream */
if (streamid > maxpstreams)		if (streamid > devep->ep_MaxPStreams)
return (XHCI_TRB_ERROR_STREAM_TYPE);		return (XHCI_TRB_ERROR_STREAM_TYPE);

sctx = XHCI_GADDR(sc, ep->qwEpCtx2 & ~0xFUL) + streamid;		sctx = XHCI_GADDR(sc, ep->qwEpCtx2 & ~0xFUL) + streamid;
		markjUnsubmitted Not Done Inline Actions I don't quite understand this line, shouldn't it be adding `streamid * sizeof(sctx)` or something like that? markj:* I don't quite understand this line, shouldn't it be adding `streamid * sizeof(*sctx)` or…
		jhbAuthorUnsubmitted Done Inline Actions Hmm, or we should be casting the result of XHCI_GADDR to the right type, yes. jhb: Hmm, or we should be casting the result of XHCI_GADDR to the right type, yes.
if (!XHCI_SCTX_0_SCT_GET(sctx->qwSctx0))		if (!XHCI_SCTX_0_SCT_GET(sctx->qwSctx0))
return (XHCI_TRB_ERROR_STREAM_TYPE);		return (XHCI_TRB_ERROR_STREAM_TYPE);

*osctx = sctx;		*osctx = sctx;

return (XHCI_TRB_ERROR_SUCCESS);		return (XHCI_TRB_ERROR_SUCCESS);
}		}

▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines	pci_xhci_cmd_set_tr(struct pci_xhci_softc *sc, uint32_t slot,
default:		default:
DPRINTF(("pci_xhci cmd set_tr invalid state %x",		DPRINTF(("pci_xhci cmd set_tr invalid state %x",
XHCI_EPCTX_0_EPSTATE_GET(ep_ctx->dwEpCtx0)));		XHCI_EPCTX_0_EPSTATE_GET(ep_ctx->dwEpCtx0)));
cmderr = XHCI_TRB_ERROR_CONTEXT_STATE;		cmderr = XHCI_TRB_ERROR_CONTEXT_STATE;
goto done;		goto done;
}		}

streamid = XHCI_TRB_2_STREAM_GET(trb->dwTrb2);		streamid = XHCI_TRB_2_STREAM_GET(trb->dwTrb2);
if (XHCI_EPCTX_0_MAXP_STREAMS_GET(ep_ctx->dwEpCtx0) > 0) {		if (devep->ep_MaxPStreams > 0) {
struct xhci_stream_ctx *sctx;		struct xhci_stream_ctx *sctx;

sctx = NULL;		sctx = NULL;
cmderr = pci_xhci_find_stream(sc, ep_ctx, streamid, &sctx);		cmderr = pci_xhci_find_stream(sc, ep_ctx, devep, streamid,
		&sctx);
if (sctx != NULL) {		if (sctx != NULL) {
assert(devep->ep_sctx != NULL);		assert(devep->ep_sctx != NULL);

devep->ep_sctx[streamid].qwSctx0 = trb->qwTrb0;		devep->ep_sctx[streamid].qwSctx0 = trb->qwTrb0;
		markjUnsubmitted Not Done Inline Actions It looks like this code is supposed to be using `sctx` instead of `devep->ep_sctx[streamid]`. markj: It looks like this code is supposed to be using `sctx` instead of `devep->ep_sctx[streamid]`.
		jhbAuthorUnsubmitted Done Inline Actions I bet this then is the other half of the bug you found above (or rather, this makes the bug you found above harmless since the incorrect pointer isn't used). I can clean this up though in a followup commit. jhb: I bet this then is the other half of the bug you found above (or rather, this makes the bug you…
		markjUnsubmitted Not Done Inline Actions Thanks. The other half will be caught eventually when we bump WARNS (arithmetic on a void pointer), but this should be cleaned up too. BTW, I think the other half of the bug is not quite harmless since pci_xhci_find_stream() checks some bits in sctx->qwSctx0, but I guess it happens to work out. markj: Thanks. The other half will be caught eventually when we bump WARNS (arithmetic on a void…
devep->ep_sctx_trbs[streamid].ringaddr =		devep->ep_sctx_trbs[streamid].ringaddr =
trb->qwTrb0 & ~0xF;		trb->qwTrb0 & ~0xF;
devep->ep_sctx_trbs[streamid].ccs =		devep->ep_sctx_trbs[streamid].ccs =
XHCI_EPCTX_2_DCS_GET(trb->qwTrb0);		XHCI_EPCTX_2_DCS_GET(trb->qwTrb0);
}		}
} else {		} else {
if (streamid != 0) {		if (streamid != 0) {
DPRINTF(("pci_xhci cmd set_tr streamid %x != 0",		DPRINTF(("pci_xhci cmd set_tr streamid %x != 0",
▲ Show 20 Lines • Show All 349 Lines • ▼ Show 20 Lines
}		}

static void		static void
pci_xhci_update_ep_ring(struct pci_xhci_softc sc, struct pci_xhci_dev_emu dev,		pci_xhci_update_ep_ring(struct pci_xhci_softc sc, struct pci_xhci_dev_emu dev,
struct pci_xhci_dev_ep devep, struct xhci_endp_ctx ep_ctx,		struct pci_xhci_dev_ep devep, struct xhci_endp_ctx ep_ctx,
uint32_t streamid, uint64_t ringaddr, int ccs)		uint32_t streamid, uint64_t ringaddr, int ccs)
{		{

if (XHCI_EPCTX_0_MAXP_STREAMS_GET(ep_ctx->dwEpCtx0) != 0) {		if (devep->ep_MaxPStreams != 0) {
devep->ep_sctx[streamid].qwSctx0 = (ringaddr & ~0xFUL) \|		devep->ep_sctx[streamid].qwSctx0 = (ringaddr & ~0xFUL) \|
(ccs & 0x1);		(ccs & 0x1);

devep->ep_sctx_trbs[streamid].ringaddr = ringaddr & ~0xFUL;		devep->ep_sctx_trbs[streamid].ringaddr = ringaddr & ~0xFUL;
devep->ep_sctx_trbs[streamid].ccs = ccs & 0x1;		devep->ep_sctx_trbs[streamid].ccs = ccs & 0x1;
ep_ctx->qwEpCtx2 = (ep_ctx->qwEpCtx2 & ~0x1) \| (ccs & 0x1);		ep_ctx->qwEpCtx2 = (ep_ctx->qwEpCtx2 & ~0x1) \| (ccs & 0x1);

DPRINTF(("xhci update ep-ring stream %d, addr %lx",		DPRINTF(("xhci update ep-ring stream %d, addr %lx",
▲ Show 20 Lines • Show All 294 Lines • ▼ Show 20 Lines	pci_xhci_device_doorbell(struct pci_xhci_softc *sc, uint32_t slot,

/* handle pending transfers */		/* handle pending transfers */
if (devep->ep_xfer->ndata > 0) {		if (devep->ep_xfer->ndata > 0) {
pci_xhci_try_usb_xfer(sc, dev, devep, ep_ctx, slot, epid);		pci_xhci_try_usb_xfer(sc, dev, devep, ep_ctx, slot, epid);
return;		return;
}		}

/* get next trb work item */		/* get next trb work item */
if (XHCI_EPCTX_0_MAXP_STREAMS_GET(ep_ctx->dwEpCtx0) != 0) {		if (devep->ep_MaxPStreams != 0) {
struct xhci_stream_ctx *sctx;		struct xhci_stream_ctx *sctx;

/*		/*
* Stream IDs of 0, 65535 (any stream), and 65534		* Stream IDs of 0, 65535 (any stream), and 65534
* (prime) are invalid.		* (prime) are invalid.
*/		*/
if (streamid == 0 \|\| streamid == 65534 \|\| streamid == 65535) {		if (streamid == 0 \|\| streamid == 65534 \|\| streamid == 65535) {
DPRINTF(("pci_xhci: invalid stream %u", streamid));		DPRINTF(("pci_xhci: invalid stream %u", streamid));
return;		return;
}		}

sctx = NULL;		sctx = NULL;
pci_xhci_find_stream(sc, ep_ctx, streamid, &sctx);		pci_xhci_find_stream(sc, ep_ctx, devep, streamid, &sctx);
if (sctx == NULL) {		if (sctx == NULL) {
DPRINTF(("pci_xhci: invalid stream %u", streamid));		DPRINTF(("pci_xhci: invalid stream %u", streamid));
return;		return;
}		}
sctx_tr = &devep->ep_sctx_trbs[streamid];		sctx_tr = &devep->ep_sctx_trbs[streamid];
ringaddr = sctx_tr->ringaddr;		ringaddr = sctx_tr->ringaddr;
ccs = sctx_tr->ccs;		ccs = sctx_tr->ccs;
trb = XHCI_GADDR(sc, sctx_tr->ringaddr & ~0xFUL);		trb = XHCI_GADDR(sc, sctx_tr->ringaddr & ~0xFUL);
▲ Show 20 Lines • Show All 1,227 Lines • Show Last 20 Lines

Fix cmp(1) to handle the case where standard streams are closed.ClosedPublicActions