shithub: riscv

Info  •  Files  •  Log  •  Branches

segment: fix read/write g->dlen race, avoid copying kernel memory, qlock

code like "return g->dlen;" is wrong as we do not hold the
qlock of the global segment. another process could come in
and override g->dlen making us return the wrong byte count.

avoid copying when we already got a kernel address (kernel memory
is the same on processes) which is the case with bread()/bwrite().
this is the same optimization that devsd does.

also avoid allocating/freeing and copying while holding the qlock.
when we copy to/from user memory, we might fault preventing
others from accessing the segment while fault handling is in
progress.

--- a/sys/src/9/port/devsegment.c

+++ b/sys/src/9/port/devsegment.c

@@ -231,7 +231,7 @@

 		if(g->s == nil)

 			error("segment not yet allocated");

 		if(g->kproc == nil){

-			qlock(&g->l);

+			eqlock(&g->l);

 			if(waserror()){

 				qunlock(&g->l);

 				nexterror();

@@ -316,6 +316,57 @@

 }

 static long

+segmentio(Globalseg *g, void *a, long n, vlong off, int wr)

+{

+	uintptr m;

+	void *b;

+

+	m = g->s->top - g->s->base;

+	if(off < 0 || off >= m){

+		if(wr)

+			error(Ebadarg);

+		return 0;

+	}

+	if(off+n > m){

+		if(wr)

+			error(Ebadarg);	

+		n = m - off;

+	}

+

+	if((uintptr)a > KZERO)

+		b = a;

+	else {

+		b = smalloc(n);

+		if(waserror()){

+			free(b);

+			nexterror();

+		}

+		if(wr)

+			memmove(b, a, n);

+	}

+

+	eqlock(&g->l);

+	if(waserror()){

+		qunlock(&g->l);

+		nexterror();

+	}

+	g->off = off + g->s->base;

+	g->data = b;

+	g->dlen = n;

+	docmd(g, wr ? Cwrite : Cread);

+	qunlock(&g->l);

+	poperror();

+

+	if(a != b){

+		if(!wr)

+			memmove(a, b, n);

+		free(b);

+		poperror();

+	}

+	return n;

+}

+

+static long

 segmentread(Chan *c, void *a, long n, vlong voff)

 {

 	Globalseg *g;

@@ -324,9 +375,9 @@

 	if(c->qid.type == QTDIR)

 		return devdirread(c, a, n, (Dirtab *)0, 0L, segmentgen);

+	g = c->aux;

 	switch(TYPE(c)){

 	case Qctl:

-		g = c->aux;

 		if(g->s == nil)

 			error("segment not yet allocated");

 		if((g->s->type&SG_TYPE) == SG_FIXED)

@@ -336,26 +387,7 @@

 			snprint(buf, sizeof(buf), "va %#p %#p\n", g->s->base, g->s->top-g->s->base);

 		return readstr(voff, a, n, buf);

 	case Qdata:

-		g = c->aux;

-		if(voff > g->s->top - g->s->base)

-			error(Ebadarg);

-		if(voff + n > g->s->top - g->s->base)

-			n = g->s->top - g->s->base - voff;

-		qlock(&g->l);

-		g->off = voff + g->s->base;

-		g->data = smalloc(n);

-		if(waserror()){

-			free(g->data);

-			qunlock(&g->l);

-			nexterror();

-		}

-		g->dlen = n;

-		docmd(g, Cread);

-		memmove(a, g->data, g->dlen);

-		free(g->data);

-		qunlock(&g->l);

-		poperror();

-		return g->dlen;

+		return segmentio(g, a, n, voff, 0);

 	default:

 		panic("segmentread");

 	}

@@ -372,9 +404,9 @@

 	if(c->qid.type == QTDIR)

 		error(Eperm);

+	g = c->aux;

 	switch(TYPE(c)){

 	case Qctl:

-		g = c->aux;

 		cb = parsecmd(a, n);

 		if(strcmp(cb->f[0], "va") == 0){

 			if(g->s != nil)

@@ -398,24 +430,7 @@

 			error(Ebadctl);

 		break;

 	case Qdata:

-		g = c->aux;

-		if(voff + n > g->s->top - g->s->base)

-			error(Ebadarg);

-		qlock(&g->l);

-		g->off = voff + g->s->base;

-		g->data = smalloc(n);

-		if(waserror()){

-			free(g->data);

-			qunlock(&g->l);

-			nexterror();

-		}

-		g->dlen = n;

-		memmove(g->data, a, g->dlen);

-		docmd(g, Cwrite);

-		free(g->data);

-		qunlock(&g->l);

-		poperror();

-		return g->dlen;

+		return segmentio(g, a, n, voff, 1);

 	default:

 		panic("segmentwrite");

 	}