diff mbox series

[4/9] bsd-user: Two helper routines oidfmt and sysctl_oldcvt

Message ID 20230210231829.39476-5-imp@bsdimp.com (mailing list archive)
State New, archived
Headers show
Series 2023 Q1 bsd-user upstreaming: bugfixes and sysctl | expand

Commit Message

Warner Losh Feb. 10, 2023, 11:18 p.m. UTC
From: Stacey Son <sson@FreeBSD.org>

oidfmt uses undocumented system call to get the type of the sysctl.
sysctl_oldcvt does the byte swapping in the data to return it to the
target.

Co-Authored-by: Sean Bruno <sbruno@FreeBSD.org>
Signed-off-by: Sean Bruno <sbruno@FreeBSD.org>
Co-Authored-by: Juergen Lock <nox@jelal.kn-bremen.de>
Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
Co-Authored-by: Raphael Kubo da Costa <rakuco@FreeBSD.org>
Signed-off-by: Raphael Kubo da Costa <rakuco@FreeBSD.org>
Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/freebsd/os-sys.c | 94 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

Comments

Richard Henderson Feb. 11, 2023, 10:17 p.m. UTC | #1
On 2/10/23 13:18, Warner Losh wrote:
> +static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
> +{
> +    switch (kind & CTLTYPE) {
> +    case CTLTYPE_INT:
> +    case CTLTYPE_UINT:
> +        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
> +        break;
> +
> +#ifdef TARGET_ABI32
> +    case CTLTYPE_LONG:
> +    case CTLTYPE_ULONG:
> +        /*
> +         * If the sysctl has a type of long/ulong but seems to be bigger than
> +         * these data types, its probably an array.  Double check that its
> +         * evenly divisible by the size of long and convert holdp to a series of
> +         * 32bit elements instead, adjusting holdlen to the new size.
> +         */
> +        if ((*holdlen > sizeof(abi_ulong)) &&
> +            ((*holdlen % sizeof(abi_ulong)) == 0)) {
> +            int array_size = *holdlen / sizeof(long);
> +            int i;
> +            if (holdp) {
> +                for (i = 0; i < array_size; i++) {
> +                    ((uint32_t *)holdp)[i] = tswap32(((long *)holdp)[i]);
> +                }
> +                *holdlen = array_size * sizeof(abi_ulong);
> +            } else {
> +                *holdlen = sizeof(abi_ulong);
> +            }
> +        } else {
> +            *(uint32_t *)holdp = tswap32(*(long *)holdp);
> +            *holdlen = sizeof(uint32_t);

This is totally confusing.  Why would it ever be an array?
Why is this section the only place we ever assign back into holdlen?

Can you point to anything similar in the freebsd source?  The whole thing is pretty hard 
to track, starting from sys/kern/kern_sysctl.c.


r~
Warner Losh Feb. 12, 2023, 4:11 a.m. UTC | #2
On Sat, Feb 11, 2023 at 3:17 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 2/10/23 13:18, Warner Losh wrote:
> > +static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
> > +{
> > +    switch (kind & CTLTYPE) {
> > +    case CTLTYPE_INT:
> > +    case CTLTYPE_UINT:
> > +        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
> > +        break;
> > +
> > +#ifdef TARGET_ABI32
> > +    case CTLTYPE_LONG:
> > +    case CTLTYPE_ULONG:
> > +        /*
> > +         * If the sysctl has a type of long/ulong but seems to be
> bigger than
> > +         * these data types, its probably an array.  Double check that
> its
> > +         * evenly divisible by the size of long and convert holdp to a
> series of
> > +         * 32bit elements instead, adjusting holdlen to the new size.
> > +         */
> > +        if ((*holdlen > sizeof(abi_ulong)) &&
> > +            ((*holdlen % sizeof(abi_ulong)) == 0)) {
> > +            int array_size = *holdlen / sizeof(long);
> > +            int i;
> > +            if (holdp) {
> > +                for (i = 0; i < array_size; i++) {
> > +                    ((uint32_t *)holdp)[i] = tswap32(((long
> *)holdp)[i]);
> > +                }
> > +                *holdlen = array_size * sizeof(abi_ulong);
> > +            } else {
> > +                *holdlen = sizeof(abi_ulong);
> > +            }
> > +        } else {
> > +            *(uint32_t *)holdp = tswap32(*(long *)holdp);
> > +            *holdlen = sizeof(uint32_t);
>
> This is totally confusing.  Why would it ever be an array?
> Why is this section the only place we ever assign back into holdlen?
>
> Can you point to anything similar in the freebsd source?  The whole thing
> is pretty hard
> to track, starting from sys/kern/kern_sysctl.c.
>

I need to understand this... I've been looking for where we export an
array, and we just don't.

I've asked the original author who said it had something to do with
different size longs. I'll
look into that a bit and get back to this.

I think we assign back into holdlen in a weird attempt adjust for the
difference of LONG between
the two. But I'm not sure that that's where we should assign.

Warner
Warner Losh Feb. 12, 2023, 5:01 p.m. UTC | #3
Hey Richard

Thanks for the very interesting question... This kept me up...

Kyle,

Please double check what I've written below to make sure I've not missed
anything.
This might well be the source of some of the weird errors we're seeing on
some
ports, but sysctl is rare enough I'm guessing that any of the overflows are
in the
end benign.

On Sat, Feb 11, 2023 at 9:11 PM Warner Losh <imp@bsdimp.com> wrote:

>
>
> On Sat, Feb 11, 2023 at 3:17 PM Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> On 2/10/23 13:18, Warner Losh wrote:
>> > +static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
>> > +{
>> > +    switch (kind & CTLTYPE) {
>> > +    case CTLTYPE_INT:
>> > +    case CTLTYPE_UINT:
>> > +        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
>> > +        break;
>> > +
>> > +#ifdef TARGET_ABI32
>> > +    case CTLTYPE_LONG:
>> > +    case CTLTYPE_ULONG:
>> > +        /*
>> > +         * If the sysctl has a type of long/ulong but seems to be
>> bigger than
>> > +         * these data types, its probably an array.  Double check that
>> its
>> > +         * evenly divisible by the size of long and convert holdp to a
>> series of
>> > +         * 32bit elements instead, adjusting holdlen to the new size.
>> > +         */
>> > +        if ((*holdlen > sizeof(abi_ulong)) &&
>> > +            ((*holdlen % sizeof(abi_ulong)) == 0)) {
>> > +            int array_size = *holdlen / sizeof(long);
>> > +            int i;
>> > +            if (holdp) {
>> > +                for (i = 0; i < array_size; i++) {
>> > +                    ((uint32_t *)holdp)[i] = tswap32(((long
>> *)holdp)[i]);
>> > +                }
>> > +                *holdlen = array_size * sizeof(abi_ulong);
>> > +            } else {
>> > +                *holdlen = sizeof(abi_ulong);
>> > +            }
>> > +        } else {
>> > +            *(uint32_t *)holdp = tswap32(*(long *)holdp);
>> > +            *holdlen = sizeof(uint32_t);
>>
>> This is totally confusing.  Why would it ever be an array?
>> Why is this section the only place we ever assign back into holdlen?
>>
>> Can you point to anything similar in the freebsd source?  The whole thing
>> is pretty hard
>> to track, starting from sys/kern/kern_sysctl.c.
>>
>
> I need to understand this... I've been looking for where we export an
> array, and we just don't.
>
> I've asked the original author who said it had something to do with
> different size longs. I'll
> look into that a bit and get back to this.
>
> I think we assign back into holdlen in a weird attempt adjust for the
> difference of LONG between
> the two. But I'm not sure that that's where we should assign.
>

OK. I understand what's going on. If you look at kern_sysctl.c
sysctl_old_ddb or
sbin/sysctl/sysctl.c show_var, you'll see that these values canbe arrays.
This code
only implements the array part for long and ulong, most likely because
that's
all that was encountered in the field.

 So the code is right, as far as it goes.... But if the value is bigger
than a long, it
will be truncated, which strikes me as a rather weird thing to do since
most longs
are for sizes of things, so I'd think it would be better to saturate.

We also adjust the length here because the host's memory requirements
are larger than tha targets. This also means that we're likely returning an
error for long/ulong fetches since the target would pass in 4 and the host
would want 8, and would return ENOMEM. There's no code to cope with
this at all, but I think there needs to be a temporary host buffer that's
then copied to the target buffer once it's converted. So I need to write
that code.

Also, this code doesn't handle the newer types that FreeBSD has grown
in the last few years: _{S,U}{8,16,32,64}. At least those are fixed between
the two different ABIs that freebsd supports (ILP32 and LP64).

Also, there's a size issue. *holdlen is a size_t, so we need to do a similar
brokering for ABI32 targets. The interface is such that we need to
read/write
this variable because that's what the kernel is doing (reading it to make
sure
it's big enough, and then writing it to the actual size).

Also (not relevant to this patch), we must not set sysctls very often. newp
needs similar treatment tooldp (except the reverse direction), but isn't
getting any of the tswaptreatment, so it's broken for long/ulong types as
well
as on powerpc which we have out-of-tree now and is the only big-endian
port we have left.

tl;dr: I think I'm going to have to do a bit of a rewrite here...

Warner
Warner Losh Feb. 12, 2023, 5:11 p.m. UTC | #4
On Sun, Feb 12, 2023 at 10:01 AM Warner Losh <imp@bsdimp.com> wrote:

> Hey Richard
>
> Thanks for the very interesting question... This kept me up...
>
> Kyle,
>
> Please double check what I've written below to make sure I've not missed
> anything.
> This might well be the source of some of the weird errors we're seeing on
> some
> ports, but sysctl is rare enough I'm guessing that any of the overflows
> are in the
> end benign.
>
> On Sat, Feb 11, 2023 at 9:11 PM Warner Losh <imp@bsdimp.com> wrote:
>
>>
>>
>> On Sat, Feb 11, 2023 at 3:17 PM Richard Henderson <
>> richard.henderson@linaro.org> wrote:
>>
>>> On 2/10/23 13:18, Warner Losh wrote:
>>> > +static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
>>> > +{
>>> > +    switch (kind & CTLTYPE) {
>>> > +    case CTLTYPE_INT:
>>> > +    case CTLTYPE_UINT:
>>> > +        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
>>> > +        break;
>>> > +
>>> > +#ifdef TARGET_ABI32
>>> > +    case CTLTYPE_LONG:
>>> > +    case CTLTYPE_ULONG:
>>> > +        /*
>>> > +         * If the sysctl has a type of long/ulong but seems to be
>>> bigger than
>>> > +         * these data types, its probably an array.  Double check
>>> that its
>>> > +         * evenly divisible by the size of long and convert holdp to
>>> a series of
>>> > +         * 32bit elements instead, adjusting holdlen to the new size.
>>> > +         */
>>> > +        if ((*holdlen > sizeof(abi_ulong)) &&
>>> > +            ((*holdlen % sizeof(abi_ulong)) == 0)) {
>>> > +            int array_size = *holdlen / sizeof(long);
>>> > +            int i;
>>> > +            if (holdp) {
>>> > +                for (i = 0; i < array_size; i++) {
>>> > +                    ((uint32_t *)holdp)[i] = tswap32(((long
>>> *)holdp)[i]);
>>> > +                }
>>> > +                *holdlen = array_size * sizeof(abi_ulong);
>>> > +            } else {
>>> > +                *holdlen = sizeof(abi_ulong);
>>> > +            }
>>> > +        } else {
>>> > +            *(uint32_t *)holdp = tswap32(*(long *)holdp);
>>> > +            *holdlen = sizeof(uint32_t);
>>>
>>> This is totally confusing.  Why would it ever be an array?
>>> Why is this section the only place we ever assign back into holdlen?
>>>
>>> Can you point to anything similar in the freebsd source?  The whole
>>> thing is pretty hard
>>> to track, starting from sys/kern/kern_sysctl.c.
>>>
>>
>> I need to understand this... I've been looking for where we export an
>> array, and we just don't.
>>
>> I've asked the original author who said it had something to do with
>> different size longs. I'll
>> look into that a bit and get back to this.
>>
>> I think we assign back into holdlen in a weird attempt adjust for the
>> difference of LONG between
>> the two. But I'm not sure that that's where we should assign.
>>
>
> OK. I understand what's going on. If you look at kern_sysctl.c
> sysctl_old_ddb or
> sbin/sysctl/sysctl.c show_var, you'll see that these values canbe arrays.
> This code
> only implements the array part for long and ulong, most likely because
> that's
> all that was encountered in the field.
>
>  So the code is right, as far as it goes.... But if the value is bigger
> than a long, it
> will be truncated, which strikes me as a rather weird thing to do since
> most longs
> are for sizes of things, so I'd think it would be better to saturate.
>
> We also adjust the length here because the host's memory requirements
> are larger than tha targets. This also means that we're likely returning an
> error for long/ulong fetches since the target would pass in 4 and the host
> would want 8, and would return ENOMEM. There's no code to cope with
> this at all, but I think there needs to be a temporary host buffer that's
> then copied to the target buffer once it's converted. So I need to write
> that code.
>
> Also, this code doesn't handle the newer types that FreeBSD has grown
> in the last few years: _{S,U}{8,16,32,64}. At least those are fixed between
> the two different ABIs that freebsd supports (ILP32 and LP64).
>
> Also, there's a size issue. *holdlen is a size_t, so we need to do a
> similar
> brokering for ABI32 targets. The interface is such that we need to
> read/write
> this variable because that's what the kernel is doing (reading it to make
> sure
> it's big enough, and then writing it to the actual size).
>

Actually, this issue isn't an issue because, modulo bugs, the callers of
sysctl_freebsd_oid() handle it.


> Also (not relevant to this patch), we must not set sysctls very often. newp
> needs similar treatment tooldp (except the reverse direction), but isn't
> getting any of the tswaptreatment, so it's broken for long/ulong types as
> well
> as on powerpc which we have out-of-tree now and is the only big-endian
> port we have left.
>
> tl;dr: I think I'm going to have to do a bit of a rewrite here...
>
> Warner
>
diff mbox series

Patch

diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
index 1676ec10f83..e3b9f168a2b 100644
--- a/bsd-user/freebsd/os-sys.c
+++ b/bsd-user/freebsd/os-sys.c
@@ -21,6 +21,100 @@ 
 #include "qemu.h"
 #include "target_arch_sysarch.h"
 
+#include <sys/sysctl.h>
+
+/*
+ * This uses the undocumented oidfmt interface to find the kind of a requested
+ * sysctl, see /sys/kern/kern_sysctl.c:sysctl_sysctl_oidfmt() (compare to
+ * src/sbin/sysctl/sysctl.c)
+ */
+static int oidfmt(int *oid, int len, char *fmt, uint32_t *kind)
+{
+    int qoid[CTL_MAXNAME + 2];
+    uint8_t buf[BUFSIZ];
+    int i;
+    size_t j;
+
+    qoid[0] = 0;
+    qoid[1] = 4;
+    memcpy(qoid + 2, oid, len * sizeof(int));
+
+    j = sizeof(buf);
+    i = sysctl(qoid, len + 2, buf, &j, 0, 0);
+    if (i) {
+        return i;
+    }
+
+    if (kind) {
+        *kind = *(uint32_t *)buf;
+    }
+
+    if (fmt) {
+        strcpy(fmt, (char *)(buf + sizeof(uint32_t)));
+    }
+    return 0;
+}
+
+/*
+ * try and convert sysctl return data for the target.
+ * Note: doesn't handle CTLTYPE_OPAQUE and CTLTYPE_STRUCT.
+ */
+static int sysctl_oldcvt(void *holdp, size_t *holdlen, uint32_t kind)
+{
+    switch (kind & CTLTYPE) {
+    case CTLTYPE_INT:
+    case CTLTYPE_UINT:
+        *(uint32_t *)holdp = tswap32(*(uint32_t *)holdp);
+        break;
+
+#ifdef TARGET_ABI32
+    case CTLTYPE_LONG:
+    case CTLTYPE_ULONG:
+        /*
+         * If the sysctl has a type of long/ulong but seems to be bigger than
+         * these data types, its probably an array.  Double check that its
+         * evenly divisible by the size of long and convert holdp to a series of
+         * 32bit elements instead, adjusting holdlen to the new size.
+         */
+        if ((*holdlen > sizeof(abi_ulong)) &&
+            ((*holdlen % sizeof(abi_ulong)) == 0)) {
+            int array_size = *holdlen / sizeof(long);
+            int i;
+            if (holdp) {
+                for (i = 0; i < array_size; i++) {
+                    ((uint32_t *)holdp)[i] = tswap32(((long *)holdp)[i]);
+                }
+                *holdlen = array_size * sizeof(abi_ulong);
+            } else {
+                *holdlen = sizeof(abi_ulong);
+            }
+        } else {
+            *(uint32_t *)holdp = tswap32(*(long *)holdp);
+            *holdlen = sizeof(uint32_t);
+        }
+        break;
+#else
+    case CTLTYPE_LONG:
+        *(uint64_t *)holdp = tswap64(*(long *)holdp);
+        break;
+    case CTLTYPE_ULONG:
+        *(uint64_t *)holdp = tswap64(*(unsigned long *)holdp);
+        break;
+#endif
+    case CTLTYPE_U64:
+    case CTLTYPE_S64:
+        *(uint64_t *)holdp = tswap64(*(uint64_t *)holdp);
+        break;
+
+    case CTLTYPE_STRING:
+        break;
+
+    default:
+        return -1;
+    }
+    return 0;
+}
+
 /* sysarch() is architecture dependent. */
 abi_long do_freebsd_sysarch(void *cpu_env, abi_long arg1, abi_long arg2)
 {