bug in zx's FBIOSCURSOR (with patch)

[Jason Thorpe <thorpej%me.com@localhost>]

I was examining all of the uses of fubyte() in the kernel for reasons completely unrelated to SUNW,leo, but while doing to noticed that I think is a bug in that driver’s FBIOSCURSOR ioctl.

At the beginning of the block of code that handles that ioctl, a local variable v is set to the mask of cursor parameters to update, and is then tested in several subsequent blocks to determine what work is to be done.

In the code that handles the FB_CUR_SETCMAP case, v is clobbered by several calls to fubyte().

This is fairly innocuous, unless you also happen to want to set the cursor shape in the same call, in which case the subsequent test to see if FB_CUR_SETSHAPE is set may fail as a result of v being clobbered while updating the cursor color map.

IMO, it’s just more straightforward to use copyin() for this, and so here is a patch that does so.  It compiles, but I can’t test it on real hardware.  If someone is able to do so, that’d be great.

Related: the following block in zx_cursor_color() looks suspicious, so I’m calling it out here… but I don’t know enough about this particular bit of hardware to know for sure:

        tmp = sc->sc_curcmap[1] | (sc->sc_curcmap[3] << 8) |
            (sc->sc_curcmap[5] << 16);
        bus_space_write_4(sc->sc_bt, sc->sc_bhzcu, zcu_data, sc->sc_curcmap[1]);

(…really?  We’re not going to use “tmp” that we just went through all the trouble to calculate?)

diff --git a/sys/dev/sbus/zx.c b/sys/dev/sbus/zx.c
index 2b04204af629..cc9df4cea081 100644
--- a/sys/dev/sbus/zx.c
+++ b/sys/dev/sbus/zx.c
@@ -475,16 +475,24 @@ zxioctl(dev_t dev, u_long cmd, void *data, int flags, struct lwp *l)
 			if (cu->cmap.index > 2 ||
 			    cu->cmap.count > 2 - cu->cmap.index)
 				return (EINVAL);
-			for (i = 0; i < cu->cmap.count; i++) {
-				if ((v = fubyte(&cu->cmap.red[i])) < 0)
-					return (EFAULT);
-				sc->sc_curcmap[i + cu->cmap.index + 0] = v;
-				if ((v = fubyte(&cu->cmap.green[i])) < 0)
-					return (EFAULT);
-				sc->sc_curcmap[i + cu->cmap.index + 2] = v;
-				if ((v = fubyte(&cu->cmap.blue[i])) < 0)
-					return (EFAULT);
-				sc->sc_curcmap[i + cu->cmap.index + 4] = v;
+
+			uint8_t red[2], green[2], blue[2];
+			const u_int cnt = cu->cmap.count;
+
+			if (cnt &&
+			    ((error = copyin(cu->cmap.red,   red,   cnt)) ||
+			     (error = copyin(cu->cmap.green, green, cnt)) ||
+			     (error = copyin(cu->cmap.blue,  blue,  cnt)))) {
+				return error;
+			}
+
+			for (i = 0; i < cnt; i++) {
+				sc->sc_curcmap[i + cu->cmap.index + 0] =
+				    red[i];
+				sc->sc_curcmap[i + cu->cmap.index + 2] =
+				    green[i];
+				sc->sc_curcmap[i + cu->cmap.index + 4] =
+				    blue[i];
 			}
 			zx_cursor_color(sc);
 		}

-- thorpej