Re: git: bb5f10e51bd5 - main - linux: implement sched_getattr()
- In reply to: John Baldwin : "Re: git: bb5f10e51bd5 - main - linux: implement sched_getattr()"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 06 Oct 2026 19:23:49 UTC
> On Oct 6, 2026, at 11:36 AM, John Baldwin <jhb@FreeBSD.org> wrote: > > On 10/6/26 10:42, Enji Cooper (yaneurabeya) wrote: >> ... >>>> + attr.size = MIN(args->size, sizeof(attr)); >> … >>>> + return (copyout(&attr, args->attr, args->size)); >>> >>> So, this lets the caller decide how much stack data they want to copy >>> out, independent of sizeof(attr). The only constraint is the >>> `args->size > PAGE_SIZE` check at the beginning of the function. >> I eliminated a duplicate calculation in the copyout which contradicted the [Linux] documented interface [1]. > > .. and introduced a kernel stack disclosure (the kind of thing we normally > have to ship an SA for). Using attr.size for the length here is the > correct solution. Yeah. I figured out that I typoed `args->size` instead of `attr.size` in this commit when I was rereading the Differential: https://reviews.freebsd.org/D60214?id=188351#inline-356758 (it took me longer to figure out what @markj was originally pointing out was wrong in this change). FWIW, I agree that the unsanitized user-provided value shouldn’t have been used. To protect against making that mistake in the future, I renamed variables to hopefully make their use clearer and make the variables a bit more unique. It helps me personally because my brain sometimes blurs details when there are a lot of commonalities with words (both written and phonetic). ... >> The interface documented by Linux allows the user to specify the amount of data to copy out (https://www.man7.org/linux/man-pages/man2/sched_getattr.2.html): >>> The fields of the sched_attr structure are as follows: >>> >>> size This field should be set to the size of the structure in >> bytes, as in sizeof(struct sched_attr). If the provided >> structure is smaller than the kernel structure, any >> additional fields are assumed to be '0'. If the provided >> structure is larger than the kernel structure, the kernel >> verifies that all additional fields are 0; if they are not, >> sched_setattr() fails with the error E2BIG and updates size >> to contain the size of the kernel structure. > > Right, so the extra bytes in the case that args->size is >> sizeof(attr) are supposed to be all zeroes already (and you > are supposed to check that in the syscall if you aren't already) > so you don't need to copy them back out again, only the > sizeof(attr) bytes including the updated size. > > This is not all that weird, and is similar to how we handle > cpuset masks which also can be differently sized than the > kernel's representation. Ok. I guess my earlier statement demonstrates how little experience I have in this area. Thank you, -Enji