about recent cgd (cryptographic driver) and ccd fixes and dk_lookup interface modifications...

Hi, it looks like the latest changes with respect to
userspace/kernelspace and dk_lookup interface in ccd/cgd
drivers:
http://cvsw
eb.netbsd.org/bsdweb.cgi/src/sys/dev/ccd.c?rev=1.120&con
tent-type=text/x-cvsweb-markup
http://c
vsweb.netbsd.org/bsdweb.cgi/src/sys/dev/cgd.c?rev=1.42.2.1&a
mp;content-type=text/x-cvsweb-markup

have beaten me in terms of talking about these issues from
my previous
experience (i.e. I too had recetnly found a problem when
using cgd on
macppc port and have subsequently "hacked up" some
thoughts/ideas on
the subject)... But for the sake of "doing things the
most optimal
way" or just for the sake of my learning of the
netbsd's kernel
internals a would like to post that message anyway with the
following
question as a prelude:

Are we sure that modifying the dk_lookup is the best option
(as
opposed to making cgd/ccd use copyinstr functionality in
their
respective ccd.c/cgd.c code - especially considering that
cgd.c
already uses copyinstr for other text fields in
"cgd_ioctl" structure
*and* that the "userspace" to
"kernelspace" conversion for the
"ci_disk" member of "cgd_ioctl" is
already done in "cgdinit" code
anyway...

And before going on with the "meat of the message"
one side-question
in relation to ccd:
"
		error = copyin(ccio->ccio_disks, cpp,
		    ccio->ccio_ndisks * sizeof(char **));
"
should it not be:
"ccio->ccio_ndisks * sizeof(char *); "
albeit practically sizeof (char **) == sizeof (char *), it
might be
more "pedantically" correct since the ccio_disks
appears to be treated
as an array of "char *" (i.e. an array of
"pointers to character"
hence ccio_disks is "char **") ... could be just
my sleepy brain after
14hrs ride on a train without a single snooze 

Anyway, here goes the message that I wanted to post in
relation to
cgd/dk_lookup and copyinstr:

Hi all,
I have a problem running cgd (cryptographic disk driver) on
macppc,
NetBSD-current.

Specifically, the following script:
"
dd if=/dev/zero of=./deleteMe bs=1m count=100
vnconfig vnd0 ./deleteMe
cgdconfig -s cgd0 /dev/vnd0c aes-cbc 128 < /dev/urandom
"
generates the following error:
"cgdconfig: ioctl: No such file or directory"

When looking at the source code of various things (cgdconfig
utility,
cgd.c driver, etc.):
in /usr/src/sbin/cgdconfig/cgdconfig.c,  the

configure_params(int fd, const char *cgd, const char *dev,
struct params *p)
calls ioctl as something like:

"
	struct cgd_ioctl ci;
	ci.ci_disk = dev;

	if (ioctl(fd, CGDIOCSET, &ci) == -1) {
"
the point being that "dev" arg (and subsequently
the "ci_disk" member
of "ci" struct) appears to be allocated in user
space.

But then, when we get to kernel space (drivers), in
/usr/src/sys/dev/cgd.c there is something like:
"
	cgd_ioctl_set(struct cgd_softc *cs, void *data, struct lwp
*l)
	{
		struct	 cgd_ioctl *ci = data;

		const char *cp;

		cp = ci->ci_disk;
"
At this stage, I think, the "cp" still holds
address from userspace...
but in the subsequent code (still in the
"cgd_ioctl_set") there is a
call to:
"
	if ((ret = dk_lookup(cp, l, &vp)) != 0)
		return ret;
"
And (in /usr/src/sys/dev/dksubr.c) the dk_lookup(const char
*path,
struct lwp *l, struct vnode **vpp)
has code like:
"
	struct nameidata nd;

	NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, path, l);
	if ((error = vn_open(&nd, FREAD | FWRITE, 0)) != 0) {
"
>From the above code, it would appear that the
"path" arg still points
to user-space, but the NDINIT indicates UIO_SYSSPACE (which
is used in
vn_open which calls namei which subsequently fails)...

When the code:
"
	NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, path, l);
"
is changed to:
"
	NDINIT(&nd, LOOKUP, FOLLOW, UIO_USERSPACE, path, l);
"
then the command:
cgdconfig -s cgd0 /dev/vnd0c aes-cbc 128 < /dev/urandom
does not generate "ioctl: No such file or
directory" error.

Of course, modifying "dk_lookup" to use
UIO_USERSPACE is not right
(dk_lookup is also used by raidframe), but rather - would it
make more
sense to use "copyinstr" for ci_disk member of
cgd_ioctl (just like
cgd.c does for ci_alg, ci_ivmethod, ci_key) prior to
callind
"dk_lookup"?

So the code would be changed from this:
"
#define MAX_KEYSIZE	1024
cgd_ioctl_set(struct cgd_softc *cs, void *data, struct lwp
*l)
{
	struct	 cgd_ioctl *ci = data;
	struct	 vnode *vp;
	int	 ret;
	size_t	 keybytes;			/* key length in bytes */
	const char *cp;
	char	 *inbuf;

	cp = ci->ci_disk;
	if ((ret = dk_lookup(cp, l, &vp)) != 0)
		return ret;

	inbuf = malloc(MAX_KEYSIZE, M_TEMP, M_WAITOK);

	if ((ret = cgdinit(cs, cp, vp, l)) != 0)
		goto bail;

	(void)memset(inbuf, 0, MAX_KEYSIZE);
	ret = copyinstr(ci->ci_alg, inbuf, 256, NULL);
	if (ret)
		goto bail;
"
to something like:
"
#define MAX_KEYSIZE	max(1024, MAXPATHLEN)
cgd_ioctl_set(struct cgd_softc *cs, void *data, struct lwp
*l)
{
	struct	 cgd_ioctl *ci = data;
	struct	 vnode *vp = NULL;
	int	 ret;
	size_t	 keybytes;			/* key length in bytes */
	char	 *inbuf = malloc(MAX_KEYSIZE, M_TEMP, M_WAITOK);

	(void)memset(inbuf, 0, MAX_KEYSIZE);
	ret = copyinstr(ci->ci_disk, inbuf, MAX_KEYSIZE, NULL);
	if (ret)
		goto bail;

	if ((ret = dk_lookup(inbuf, l, &vp)) != 0)
		goto bail;

	if ((ret = cgdinit(cs, inbuf, vp, l)) != 0)
		goto bail;

	(void)memset(inbuf, 0, MAX_KEYSIZE);
	ret = copyinstr(ci->ci_alg, inbuf, 256, NULL);
	if (ret)
		goto bail;
"
and in bail label:
instead of
"
		(void)vn_close(vp, FREAD|FWRITE, l->l_cred, l);
"
have:
"
	if (vp != NULL)
		(void)vn_close(vp, FREAD|FWRITE, l->l_cred, l);
"
and because "cgdinit" would already be called with
kernel-spaced
"cpath" (after the aforementioned code
modifications) then, perhaps
one may delete (?) the following code in
static int cgdinit(struct cgd_softc *cs, const char *cpath,
struct
vnode *vp, struct lwp *l)
"
	tmppath = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
	ret = copyinstr(cpath, tmppath, MAXPATHLEN,
&cs->sc_tpathlen);
	if (ret)
		goto bail;
	cs->sc_tpath = malloc(cs->sc_tpathlen, M_DEVBUF,
M_WAITOK);
	memcpy(cs->sc_tpath, tmppath, cs->sc_tpathlen);
	memcpy(cs->sc_tpath, cpath, cs->sc_tpathlen);
"
and
"
	free(tmppath, M_TEMP);
	if (ret && cs->sc_tpath)
		free(cs->sc_tpath, M_DEVBUF);
"
and in
cgd_ioctl_clr(struct cgd_softc *cs, void *data, struct lwp
*l) may one
delete the following?
"
	free(cs->sc_tpath, M_DEVBUF);
"
I am probably missing something (e.g. the reason to use
malloc with
M_DEVBUF in one place [for sc_tpath] and M_TEMP in other
places... - I
don't know anything, really, about kernel in general and
cgd
specifically...)

Speaking more in terms of cgd.c - does anyone know how is
"sc_tpath"
(a member of "cgd_softc" structure) used at the
moment? And may it be
thrown away? - you see - I am missing something 

Kind regards
Leon