Download raw body.
Maybe strlen is unnecessary in kern_unveil.c
Sebastien Marie <semarie@kapouay.eu.org> writes:
> Bob Beck <beck@obtuse.com> 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);
}
Maybe strlen is unnecessary in kern_unveil.c