On Sun, Feb 12, 2023 at 10:01 AM Warner Losh <i...@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 <i...@bsdimp.com> wrote: > >> >> >> On Sat, Feb 11, 2023 at 3:17 PM Richard Henderson < >> richard.hender...@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 >