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

[Warner Losh <imp@bsdimp.com>]

[-- Attachment #1: Type: text/plain, Size: 4892 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 6866 bytes --]