From: Sebastien Marie Subject: Re: Maybe strlen is unnecessary in kern_unveil.c To: Bob Beck Cc: Christian Schulte , tech@openbsd.org Date: Wed, 19 Nov 2025 17:54:17 +0100 Sebastien Marie writes: > Bob Beck writes: > >> like ah. no? >> >> the point of it being named "size" and not "len" is to make it clear it is the size of the allocation and not the length of a string. >> >> renaming it to len makes it confusing, which is only partly mitigated by a comment. this is not a helpful visit to the bikeshed paint store > > my point is the following code panic, because we are currently passing > cn_nameptr which isn't a NUL terminated string. > > But I could rework the diff to ensure that we always pass NUL terminated > string (and not the opposite). Here a new diff. it uses size (and not len) and still ensure that passed name is properly NUL terminated. for simplicity, I introduced unveil_namelookup_ndp() to wrap malloc/free dance around unveil_namelookup(). Regards. -- Sebastien Marie diff --git a/sys/kern/kern_unveil.c b/sys/kern/kern_unveil.c index b59b53edb7..c5cae96214 100644 --- a/sys/kern/kern_unveil.c +++ b/sys/kern/kern_unveil.c @@ -112,11 +112,11 @@ } int -unveil_add_name_unlocked(struct unveil *uv, char *name, u_char flags) +unveil_add_name_unlocked(struct unveil *uv, char *name, size_t namesize, u_char flags) { struct unvname *unvn; - unvn = unvname_new(name, strlen(name) + 1, flags); + unvn = unvname_new(name, namesize, flags); if (RBT_INSERT(unvname_rbt, &uv->uv_names, unvn) != NULL) { /* Name already present. */ unvname_delete(unvn); @@ -128,18 +128,18 @@ } int -unveil_add_name(struct unveil *uv, char *name, u_char flags) +unveil_add_name(struct unveil *uv, char *name, size_t namesize, u_char flags) { int ret; rw_enter_write(&uv->uv_lock); - ret = unveil_add_name_unlocked(uv, name, flags); + ret = unveil_add_name_unlocked(uv, name, namesize, flags); rw_exit_write(&uv->uv_lock); return ret; } struct unvname * -unveil_namelookup(struct unveil *uv, char *name) +unveil_namelookup(struct unveil *uv, char *name, size_t namesize) { struct unvname n, *ret = NULL; @@ -151,7 +151,7 @@ KASSERT(uv->uv_vp != NULL); n.un_name = name; - n.un_namesize = strlen(name) + 1; + n.un_namesize = namesize; ret = RBT_FIND(unvname_rbt, &uv->uv_names, &n); @@ -163,6 +163,24 @@ return ret; } +struct unvname * +unveil_namelookup_ndp(struct unveil *uv, struct nameidata *ndp) +{ + struct unvname *ret; + char *name; + size_t namesize = ndp->ni_cnd.cn_namelen + 1; + + name = malloc(namesize, M_TEMP, M_WAITOK); + memcpy(name, ndp->ni_cnd.cn_nameptr, ndp->ni_cnd.cn_namelen); + name[ndp->ni_cnd.cn_namelen] = '\0'; + + ret = unveil_namelookup(uv, name, namesize); + + free(name, M_TEMP, namesize); + + return ret; +} + void unveil_destroy(struct process *ps) { @@ -222,7 +240,8 @@ rw_enter_read(&from->uv_lock); RBT_FOREACH_SAFE(unvn, unvname_rbt, &from->uv_names, next) { if (unveil_add_name_unlocked(&child->ps_uvpaths[i], - unvn->un_name, unvn->un_flags)) + unvn->un_name, unvn->un_namesize, + unvn->un_flags)) child->ps_uvncount++; } rw_exit_read(&from->uv_lock); @@ -414,6 +433,8 @@ int directory_add; int ret = EINVAL; u_char flags; + char *name; + size_t namesize; KASSERT(ISSET(ndp->ni_cnd.cn_flags, HASBUF)); /* must have SAVENAME */ @@ -473,8 +494,7 @@ */ if (!directory_add) { struct unvname *tname; - if ((tname = unveil_namelookup(uv, - ndp->ni_cnd.cn_nameptr)) != NULL) { + if ((tname = unveil_namelookup_ndp(uv, ndp)) != NULL) { DPRINTF("unveil: %s(%d): changing flags for %s" "in vnode %p, uvcount %d\n", pr->ps_comm, pr->ps_pid, tname->un_name, vp, @@ -512,13 +532,21 @@ goto done; } - if (unveil_add_name(uv, ndp->ni_cnd.cn_nameptr, flags)) + namesize = ndp->ni_cnd.cn_namelen + 1; + name = malloc(namesize, M_TEMP, M_WAITOK); + memcpy(name, ndp->ni_cnd.cn_nameptr, ndp->ni_cnd.cn_namelen); + name[ndp->ni_cnd.cn_namelen] = '\0'; + + if (unveil_add_name(uv, name, namesize, flags)) pr->ps_uvncount++; ret = 0; - DPRINTF("unveil: %s(%d): added name %s beneath %s vnode %p," + free(name, M_TEMP, namesize); + + DPRINTF("unveil: %s(%d): added name %.*s beneath %s vnode %p," " uvcount %d\n", - pr->ps_comm, pr->ps_pid, ndp->ni_cnd.cn_nameptr, + pr->ps_comm, pr->ps_pid, (int)ndp->ni_cnd.cn_namelen, + ndp->ni_cnd.cn_nameptr, uv->uv_flags ? "unrestricted" : "restricted", vp, vp->v_uvcount); @@ -702,9 +730,9 @@ } /* directory and flags match, success */ - DPRINTF("unveil: %s(%d): matched directory \"%s\" at vnode %p\n", - pr->ps_comm, pr->ps_pid, ni->ni_cnd.cn_nameptr, - uv->uv_vp); + DPRINTF("unveil: %s(%d): matched directory \"%.*s\" at vnode %p\n", + pr->ps_comm, pr->ps_pid, (int)ni->ni_cnd.cn_namelen, + ni->ni_cnd.cn_nameptr, uv->uv_vp); return (0); } @@ -717,18 +745,20 @@ goto done; } - if ((tname = unveil_namelookup(uv, ni->ni_cnd.cn_nameptr)) == NULL) { - DPRINTF("unveil: %s(%d) no match for terminal '%s' in " + if ((tname = unveil_namelookup_ndp(uv, ni)) == NULL) { + DPRINTF("unveil: %s(%d) no match for terminal '%.*s' in " "directory vnode %p\n", pr->ps_comm, pr->ps_pid, - ni->ni_cnd.cn_nameptr, ni->ni_dvp); + (int)ni->ni_cnd.cn_namelen, ni->ni_cnd.cn_nameptr, + ni->ni_dvp); /* no specific name, so check unveil directory flags */ if (!unveil_flagmatch(ni, uv->uv_flags)) { DPRINTF("unveil: %s(%d) terminal " - "'%s' flags mismatch in directory " + "'%.*s' flags mismatch in directory " "vnode %p\n", pr->ps_comm, pr->ps_pid, + (int)ni->ni_cnd.cn_namelen, ni->ni_cnd.cn_nameptr, ni->ni_dvp); /* @@ -748,9 +778,9 @@ goto done; } /* directory flags match, success */ - DPRINTF("unveil: %s(%d): matched \"%s\" underneath vnode %p\n", - pr->ps_comm, pr->ps_pid, ni->ni_cnd.cn_nameptr, - uv->uv_vp); + DPRINTF("unveil: %s(%d): matched \"%.*s\" underneath vnode %p\n", + pr->ps_comm, pr->ps_pid, (int)ni->ni_cnd.cn_namelen, + ni->ni_cnd.cn_nameptr, uv->uv_vp); return (0); } @@ -775,9 +805,10 @@ */ for (uv = ni->ni_unveil_match; uv != NULL; uv = nuv) { if (unveil_flagmatch(ni, uv->uv_flags)) { - DPRINTF("unveil: %s(%d): matched \"%s\" underneath/at " + DPRINTF("unveil: %s(%d): matched \"%.*s\" underneath/at " "vnode %p\n", pr->ps_comm, pr->ps_pid, - ni->ni_cnd.cn_nameptr, uv->uv_vp); + (int)ni->ni_cnd.cn_namelen, ni->ni_cnd.cn_nameptr, + uv->uv_vp); return (0); }