net-snmp-coders

libkvm usage on FreeBSD

[Mark J. <ma...@fr...>]

I'm quite new to net-snmp, so apologies in advance if this is the wrong
place to discuss patches.

Currently, when one runs snmpd on FreeBSD, it keeps /dev/mem and
/dev/kmem open.  This is a result of using kvm_openfiles() to get a kvm
descriptor, which is used to implement various MIBs (via kvm_getprocs(),
kvm_getswapinfo(), etc.).  However, the libkvm interfaces used by
net-snmp don't actually require /dev/mem or /dev/kmem - they are all
implemented using sysctls.

I would like to have snmpd not hold /dev/(k)mem open.  I can of course
compile using --without-kmem-usage, but that disables the use of libkvm,
so all of that code (used to implement the hostres MIB, for instance)
would need to be rewritten.  However, it turns out that one can ask
kvm_openfiles() to not open /dev/mem and /dev/kmem, by passing
"/dev/null" as the path.  In fact, snmpd already contains code to do
this, but it's only done in a fallback path in init_kmem().

I would like to introduce the patch below, which gets compiled when
--without-kmem-usage is specified.  In this case, snmpd will still use
libkvm, but won't open /dev/(k)mem.  In my testing so far, this works
perfectly.  Does anyone have any thoughts on this patch/approach?  Would
the net-snmp project be willing to accept the patch?  Thank you in
advance for any feedback or guidance.

commit 1c7881da5963508bcbcbd4ac44a60192fec4f7d8
Author: Mark Johnston <ma...@Fr...>
Date:   Thu Apr 4 16:34:26 2024 -0400

    snmpd: Enable the use of libkvm without kmem access on FreeBSD

diff --git a/agent/kernel.c b/agent/kernel.c
index 285a603a77..164a6a8ca2 100644
--- a/agent/kernel.c
+++ b/agent/kernel.c
@@ -259,7 +259,37 @@ free_kmem(void)
         kmem = -1;
     }
 }
+#elif defined(__FreeBSD__)
+kvm_t *kd;
+
+/**
+ * Initialize the libkvm descriptor. On FreeBSD we can use most of libkvm
+ * without requiring /dev/kmem access.  Only kvm_nlist() and kvm_read() need
+ * that, and we don't use them.
+ *
+ * @return TRUE upon success; FALSE upon failure.
+ */
+int
+init_kmem(const char *file)
+{
+    char err[4096];
 
+    kd = kvm_openfiles(NULL, "/dev/null", NULL, O_RDONLY, err);
+    if (!kd) {
+        snmp_log(LOG_CRIT, "init_kmem: kvm_openfiles failed: %s\n", err);
+        return FALSE;
+    }
+    return TRUE;
+}
+
+void
+free_kmem(void)
+{
+    if (kd != NULL) {
+        (void)kvm_close(kd);
+        kd = NULL;
+    }
+}
 #else
 int
 init_kmem(const char *file)

Re: libkvm usage on FreeBSD

[Bart V. A. <bva...@ac...>]

On 4/12/24 06:44, Mark Johnston wrote:
> I would like to introduce the patch below, which gets compiled when
> --without-kmem-usage is specified.  In this case, snmpd will still use
> libkvm, but won't open /dev/(k)mem.  In my testing so far, this works
> perfectly.  Does anyone have any thoughts on this patch/approach?  Would
> the net-snmp project be willing to accept the patch?  Thank you in
> advance for any feedback or guidance.

What privileges are required to call kvm_open() on FreeBSD? Are
the same privileges required as for opening /dev/kmem or not? If not,
has it been considered to make init_kmem() call kvm_open() on FreeBSD
independent of whether or not --without-kmem-usage has been specified?

Thanks,

Bart.

Re: libkvm usage on FreeBSD

[Mark J. <ma...@fr...>]

On Fri, Apr 12, 2024 at 07:13:41AM -0700, Bart Van Assche wrote:
> On 4/12/24 06:44, Mark Johnston wrote:
> > I would like to introduce the patch below, which gets compiled when
> > --without-kmem-usage is specified.  In this case, snmpd will still use
> > libkvm, but won't open /dev/(k)mem.  In my testing so far, this works
> > perfectly.  Does anyone have any thoughts on this patch/approach?  Would
> > the net-snmp project be willing to accept the patch?  Thank you in
> > advance for any feedback or guidance.
> 
> What privileges are required to call kvm_open() on FreeBSD? Are
> the same privileges required as for opening /dev/kmem or not?

The only privileges required are those needed to open the file.  So,
kvm_openfiles(NULL, NULL, NULL, O_RDONLY, err) requires root privileges,
because it tries to open /dev/mem.  If I change the second parameter to
"/dev/null", then any user can call it successfully, and most of the
kvm_* functions will still work as expected.

> If not,
> has it been considered to make init_kmem() call kvm_open() on FreeBSD
> independent of whether or not --without-kmem-usage has been specified?

Assuming I don't misunderstand, that's effectively what my patch does -
it just does so in a separate implementation of init_kmem().

Do you mean that on FreeBSD we should always perform an unprivileged
kvm_openfile() call, no matter whether --without-kmem-usage is
specified?

Re: libkvm usage on FreeBSD

[Bart V. A. <bva...@ac...>]

On 4/12/24 7:29 AM, Mark Johnston wrote:
> Do you mean that on FreeBSD we should always perform an unprivileged
> kvm_openfile() call, no matter whether --without-kmem-usage is
> specified?

Yes, that's what I'm proposing. If someone disagrees, please share your
opinion now.

Thanks,

Bart.

Re: libkvm usage on FreeBSD

[Mark J. <ma...@fr...>]

On Fri, Apr 12, 2024 at 03:51:51PM -0700, Bart Van Assche wrote:
> On 4/12/24 7:29 AM, Mark Johnston wrote:
> > Do you mean that on FreeBSD we should always perform an unprivileged
> > kvm_openfile() call, no matter whether --without-kmem-usage is
> > specified?
> 
> Yes, that's what I'm proposing. If someone disagrees, please share your
> opinion now.

I can't see any reason not to go ahead with that, for what it's worth.
I'm confident that none of the existing code in net-snmp needs /dev/kmem
access on FreeBSD, and it shouldn't be required by new code.

Re: libkvm usage on FreeBSD

[Mark J. <ma...@fr...>]

On Fri, Apr 12, 2024 at 03:51:51PM -0700, Bart Van Assche wrote:
> On 4/12/24 7:29 AM, Mark Johnston wrote:
> > Do you mean that on FreeBSD we should always perform an unprivileged
> > kvm_openfile() call, no matter whether --without-kmem-usage is
> > specified?
> 
> Yes, that's what I'm proposing. If someone disagrees, please share your
> opinion now.

I spent some more time testing this and hit a small problem with the
IPv6-TCP MIB.  The implementation doesn't compile on FreeBSD (it
references a non-existent field in struct inpcb) and so is disabled in
the FreeBSD ports tree, so I didn't catch it earlier when testing a
patch to avoid using /dev/kmem.

The implementation of var_tcp6() and var_udp6() used on FreeBSD isn't
quite right; it uses the wrong structures to access the output of the
net.inet.{tcp,udp}.pcblist sysctls, and var_tcp6() tries to use kmem
access to get the TCP state of the matched connection, which at least on
FreeBSD isn't necessary (net.inet.tcp.pcblist exports that field
directly via struct xtcpcb).

I wrote the patch below to fix this and manually verified the MIB values
on a test system.

commit f131c5b7dd599cf6c3a15b372075d299262cbfee
Author: Mark Johnston <ma...@Fr...>
Date:   Tue Apr 30 10:23:25 2024 -0400

    Fix var_udp6 and var_tcp6 on FreeBSD

diff --git a/agent/mibgroup/mibII/ipv6.c b/agent/mibgroup/mibII/ipv6.c
index f44568c9fa..1ec05efce2 100644
--- a/agent/mibgroup/mibII/ipv6.c
+++ b/agent/mibgroup/mibII/ipv6.c
@@ -5,9 +5,6 @@
 
 #include <net-snmp/net-snmp-config.h>
 #include <net-snmp/net-snmp-features.h>
-/* For FreeBSD */
-#define _WANT_INPCB 1
-#define _WANT_TCPCB 1
 #include <sys/types.h>
 #include <sys/socket.h>
 #ifdef HAVE_SYS_IOCTL_H
@@ -1513,8 +1510,10 @@ var_udp6(register struct variable * vp,
     int             result;
     int             i, j;
     caddr_t         p;
-#if defined(openbsd4) || defined(freebsd3)
+#if defined(openbsd4)
     static struct inpcb in6pcb, savpcb;
+#elif defined(freebsd3)
+    static struct xinpcb in6pcb, savpcb;
 #else
     static struct in6pcb in6pcb, savpcb;
 #endif
@@ -1614,8 +1613,7 @@ var_udp6(register struct variable * vp,
         DEBUGMSGTL(("mibII/ipv6", "looping: p=%p\n", p));
 
 #if defined(freebsd3)
-	/* To do: fill in in6pcb properly. */
-	memset(&in6pcb, 0, sizeof(in6pcb));
+	in6pcb = *(struct xinpcb *) xig;
 #elif defined(darwin)
 	in6pcb = ((struct xinpcb *) xig)->xi_inp;
 #else
@@ -2108,12 +2106,18 @@ var_tcp6(register struct variable * vp,
     int             result;
     int             i, j;
     caddr_t         p;
-#if defined(openbsd4) || defined(freebsd3)
+#if defined(openbsd4)
     static struct inpcb in6pcb, savpcb;
+#elif defined(freebsd3)
+    static struct xinpcb in6pcb;
+    static int      savstate;
 #else
     static struct in6pcb in6pcb, savpcb;
 #endif
+#if !defined(freebsd3)
     struct tcpcb    tcpcb;
+#endif
+    int             state;
     int             found, savnameLen;
 #if defined(__NetBSD__) && __NetBSD_Version__ >= 106250000 || defined(openbsd4)	/*1.6Y*/
     struct inpcbtable tcbtable;
@@ -2208,8 +2212,7 @@ var_tcp6(register struct variable * vp,
         DEBUGMSGTL(("mibII/ipv6", "looping: p=%p\n", p));
 
 #if defined(freebsd3)
-	/* To do: fill in in6pcb properly. */
-	memset(&in6pcb, 0, sizeof(in6pcb));
+	in6pcb = ((struct xtcpcb *) xig)->xt_inp;
 #elif defined(dragonfly)
 	in6pcb = xtp->xt_inp;
 #elif defined(darwin)
@@ -2294,7 +2297,11 @@ var_tcp6(register struct variable * vp,
 #endif
         result = snmp_oid_compare(name, *length, newname, j);
         if (exact && (result == 0)) {
+#if defined(freebsd3)
+                savstate = ((struct xtcpcb *) xig)->t_state;
+#else
                 memcpy(&savpcb, &in6pcb, sizeof(savpcb));
+#endif
                 savnameLen = j;
                 memcpy(savname, newname, j * sizeof(oid));
                 found++;
@@ -2305,7 +2312,11 @@ var_tcp6(register struct variable * vp,
              */
             if ((savnameLen == 0) ||
               (snmp_oid_compare(savname, savnameLen, newname, j) > 0)) {
+#if defined(freebsd3)
+                savstate = ((struct xtcpcb *) xig)->t_state;
+#else
                 memcpy(&savpcb, &in6pcb, sizeof(savpcb));
+#endif
                 savnameLen = j;
                 memcpy(savname, newname, j * sizeof(oid));
                 found++;
@@ -2344,19 +2355,27 @@ var_tcp6(register struct variable * vp,
         return NULL;
     *length = savnameLen;
     memcpy((char *) name, (char *) savname, *length * sizeof(oid));
+#if defined(freebsd3)
+    state = savstate;
+#elif defined(__NetBSD__) && __NetBSD_Version__ >= 999010400
     memcpy(&in6pcb, &savpcb, sizeof(savpcb));
-#if defined(__NetBSD__) && __NetBSD_Version__ >= 999010400
     if (!NETSNMP_KLOOKUP(in6pcb.in6p_pcb.inp_ppcb, (char *) &tcpcb, sizeof(tcpcb))) {
         DEBUGMSGTL(("mibII/ipv6", "klookup fail for tcb6.tcpcb at %p\n",
                     in6pcb.in6p_pcb.inp_ppcb));
+	found = 0;
+	return NULL;
+    }
+    state = (int)tcpcb.t_state;
 #else
+    memcpy(&in6pcb, &savpcb, sizeof(savpcb));
     if (!NETSNMP_KLOOKUP(in6pcb.inp_ppcb, (char *) &tcpcb, sizeof(tcpcb))) {
 	DEBUGMSGTL(("mibII/ipv6", "klookup fail for tcb6.tcpcb at %p\n",
 		    in6pcb.inp_ppcb));
-#endif
 	found = 0;
 	return NULL;
     }
+    state = (int)tcpcb.t_state;
+#endif
     *write_method = 0;
     *var_len = sizeof(long);    /* default to 'long' results */
 
@@ -2368,7 +2387,7 @@ var_tcp6(register struct variable * vp,
     DEBUGMSGTL(("mibII/ipv6", "magic=%d\n", vp->magic));
     switch (vp->magic) {
     case IPV6TCPCONNSTATE:
-        long_return = mapTcpState((int)tcpcb.t_state);
+        long_return = mapTcpState(state);
         return (u_char *) & long_return;
     default:
         break;

Re: libkvm usage on FreeBSD

[Bart V. A. <bva...@ac...>]

On 4/30/24 10:53 AM, Mark Johnston wrote:
> Fix var_udp6 and var_tcp6 on FreeBSD
 > [ ... ]

This patch has been applied on the V5-9-patches and master branches.
Thanks for the patch!

Bart.

Re: libkvm usage on FreeBSD

[Mark J. <ma...@fr...>]

On Fri, Apr 12, 2024 at 03:51:51PM -0700, Bart Van Assche wrote:
> On 4/12/24 7:29 AM, Mark Johnston wrote:
> > Do you mean that on FreeBSD we should always perform an unprivileged
> > kvm_openfile() call, no matter whether --without-kmem-usage is
> > specified?
> 
> Yes, that's what I'm proposing. If someone disagrees, please share your
> opinion now.

Hi Bart,

Were you planning to make a change along these lines?  Would it be
helpful for me to submit a patch?  A few of us have been testing snmpd
with my original patch (to tell libkvm not to open /dev/kmem etc.) for a
while now with no issues.

Thanks,
-Mark

Re: libkvm usage on FreeBSD

[Bart V. A. <bva...@ac...>]

On 6/10/24 13:27, Mark Johnston wrote:
> Would it be helpful for me to submit a patch?  A few of us have been
> testing snmpd with my original patch (to tell libkvm not to open
> /dev/kmem etc.) for a while now with no issues.
A patch definitely would be welcome.

Thanks,

Bart.

Re: libkvm usage on FreeBSD

[Mark J. <ma...@fr...>]

On Mon, Jun 10, 2024 at 03:51:33PM -0700, Bart Van Assche wrote:
> 
> On 6/10/24 13:27, Mark Johnston wrote:
> > Would it be helpful for me to submit a patch?  A few of us have been
> > testing snmpd with my original patch (to tell libkvm not to open
> > /dev/kmem etc.) for a while now with no issues.
> A patch definitely would be welcome.

The patch below implements your suggestion.  That is, init_kmem() gets a
libkvm handle without opening /dev/kmem, no matter whether --with-kmem-usage
or --without-kmem-usage was specified at compile time.

Thinking about this more, users might still want --with-kmem-usage to
cause snmpd to open /dev/kmem, in the case where they have custom MIBs
implementations which require the use of klookup().  With this patch,
it's impossible to use klookup() on FreeBSD.  I believe this is fine for
the code shipped with net-snmp, but it might break some custom 3rd-party
MIBs.  I'm not too concerned about this, as the official FreeBSD
net-snmp package will be compiled with --without-kmem-usage, but perhaps
it still makes sense to leave an escape hatch.

commit 304f8cf7f176920cb689d237f612c9a25cd14e84
Author: Mark Johnston <ma...@Fr...>
Date:   Thu Apr 4 16:34:26 2024 -0400

    snmpd: Always open libkvm in "safe mode" on FreeBSD
    
    By specifying /dev/null as the path to kvm_openfiles(), we can get a
    libkvm descriptor which does not hold /dev/kmem open.  None of the code
    shipped with net-snmp needs a /dev/kmem handle.
    
    Make this change for both the NETSNMP_NO_KMEM_USAGE and
    !NETSNMP_NO_KMEM_USAGE cases, per a suggestion from
    Bart Van Assche <bva...@ac...>.

diff --git a/agent/kernel.c b/agent/kernel.c
index 9a6d22592c..671e5244fc 100644
--- a/agent/kernel.c
+++ b/agent/kernel.c
@@ -44,7 +44,7 @@
 #include "kernel.h"
 #include <net-snmp/agent/ds_agent.h>
 
-#if defined(HAVE_KVM_H) && !defined(NETSNMP_NO_KMEM_USAGE)
+#if defined(HAVE_KVM_H) && !defined(NETSNMP_NO_KMEM_USAGE) && !defined(__FreeBSD__)
 kvm_t *kd;
 
 /**
@@ -130,7 +130,7 @@ free_kmem(void)
     }
 }
 
-#elif defined(HAVE_NLIST_H) && !defined(__linux__) &&   \
+#elif defined(HAVE_NLIST_H) && !defined(__linux__) && !defined(__FreeBSD__) && \
     !defined(NETSNMP_NO_KMEM_USAGE)
 
 static off_t    klseek(off_t);
@@ -252,7 +252,48 @@ free_kmem(void)
         kmem = -1;
     }
 }
+#elif defined(__FreeBSD__)
+kvm_t *kd;
+
+/**
+ * Initialize the libkvm descriptor. On FreeBSD we can use most of libkvm
+ * without requiring /dev/kmem access.  Only kvm_nlist() and kvm_read() need
+ * that, and we don't use them.
+ *
+ * @return TRUE upon success; FALSE upon failure.
+ */
+int
+init_kmem(const char *file)
+{
+    char err[4096];
+
+    kd = kvm_openfiles(NULL, "/dev/null", NULL, O_RDONLY, err);
+    if (!kd) {
+        snmp_log(LOG_CRIT, "init_kmem: kvm_openfiles failed: %s\n", err);
+        return FALSE;
+    }
+    return TRUE;
+}
 
+/**
+ * A stub to return failure to any attempt to read kernel memory.  Our
+ * libkvm handle doesn't enable /dev/kmem access.  MIB implementations should
+ * use unprivileged to fetch information about the system.
+ */
+int
+klookup(unsigned long off, void *target, size_t siz)
+{
+    return 0;
+}
+
+void
+free_kmem(void)
+{
+    if (kd != NULL) {
+        (void)kvm_close(kd);
+        kd = NULL;
+    }
+}
 #else
 int
 init_kmem(const char *file)

Re: libkvm usage on FreeBSD

[Bart V. A. <bva...@ac...>]

On 6/11/24 7:04 AM, Mark Johnston wrote:
> commit 304f8cf7f176920cb689d237f612c9a25cd14e84
> Author: Mark Johnston <ma...@Fr...>
> Date:   Thu Apr 4 16:34:26 2024 -0400
> 
>      snmpd: Always open libkvm in "safe mode" on FreeBSD

This patch has been applied on the V5-9-patches and master branches.
Thanks for the patch!

Bart.