Re: [PATCH v4 2/4] Fix alignment when reading core notes
On Tue, 2019-12-24 at 06:57 +, Taylor R Campbell wrote: > > Date: Tue, 24 Dec 2019 07:52:14 +0100 > > From: Micha� Górny > > > > On Mon, 2019-12-23 at 16:17 +, Christos Zoulas wrote: > > > In article <20191222164104.165346-2-mgo...@gentoo.org>, > > > Micha� Górny wrote: > > > > Both desc and note header needs to be aligned. Therefore, we need > > > > to realign after skipping past desc as well. > > > > > > Should probably be done with a standard alignment macro? > > > > Could you be more specific? I don't see any macro that would accept > > actual alignment value. Well, besides STACK_ALIGN() but that seems to > > do some weird casting, so I presume it's not semantically correct here. > > Instead of > > + offset = ((offset + core_hdr.p_align - 1) > + / core_hdr.p_align) * core_hdr.p_align; > > do > > + offset = roundup(offset, core_hdr.p_align); Thanks. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 2/4] Fix alignment when reading core notes
> Date: Tue, 24 Dec 2019 07:52:14 +0100 > From: Micha� Górny > > On Mon, 2019-12-23 at 16:17 +, Christos Zoulas wrote: > > In article <20191222164104.165346-2-mgo...@gentoo.org>, > > Micha� Górny wrote: > > > Both desc and note header needs to be aligned. Therefore, we need > > > to realign after skipping past desc as well. > > > > Should probably be done with a standard alignment macro? > > Could you be more specific? I don't see any macro that would accept > actual alignment value. Well, besides STACK_ALIGN() but that seems to > do some weird casting, so I presume it's not semantically correct here. Instead of + offset = ((offset + core_hdr.p_align - 1) + / core_hdr.p_align) * core_hdr.p_align; do + offset = roundup(offset, core_hdr.p_align);
Re: [PATCH v4 2/4] Fix alignment when reading core notes
On Mon, 2019-12-23 at 16:17 +, Christos Zoulas wrote: > In article <20191222164104.165346-2-mgo...@gentoo.org>, > Micha� Górny wrote: > > Both desc and note header needs to be aligned. Therefore, we need > > to realign after skipping past desc as well. > > Should probably be done with a standard alignment macro? > Could you be more specific? I don't see any macro that would accept actual alignment value. Well, besides STACK_ALIGN() but that seems to do some weird casting, so I presume it's not semantically correct here. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Synchronization protocol around TODR / time-related variables
As you may have noticed, I've been doing a significant amount of tidying up in the i2c code, specifically getting rid of polled mode access as much as possible, and auditing everything for "accesses i2c in hard interrupt context" (which is not safe). I am mostly complete with this task, but there is a notable exception: RTC (Real Time Clock) drivers. The RTC drivers provide back-ends for the TODR (Time Of Day Register) subsystem. The TODR subsystem has 3 basic functions: -- inittodr(): Initializes the in-memory copy of the system time from one of 2 sources: the last-modified time of the root file system, or the value from the back-end RTC. It is called at boot time, and when the system wakes up after a sleep / hibernate. -- resettodr(): Writes the current in-memory copy of the system time to the back-end RTC. Called from cpu_reboot(), from the Xen "push our time to hypervisor" callout, and from settime1() (a back-end for clock_settime(), etc.) -- Wrappers around the back-end RTC drivers (todr_gettime(), todr_settime()) that take care of translating the in-memory representation to/from one of a couple of different formats that hardware prefers. None of these modify any global state worth worrying about (inittodr() sets "timeset" to true, and calls tc_setclock(), which has real synchronization internally), other than the back-end hardware. todr_gettime() and todr_settime() are ONLY called by inittodr() and resettodr(), and so they could (should probably?) be made static. It seems reasonable that todr_gettime() and todr_settime() should have some serialization here -- for example, when suspending (where wake-up would later call inittodr()), you want to make sure that any in-progress clock_settime() calls, etc. have completed before proceeding. Without doing so, you run the risk of deadlocking on e.g. an i2c bus lock (polled-mode operation must die). I'm thinking that I'd do the following: -- todr_lock() -- acquires a lock on the TODR subsystem. External users would be suspend code that grabs it on the way down. -- todr_unlock() -- releases the TODR lock. -- inittodr_locked() -- External users that have previously called todr_lock() would use inittodr_locked() instead of inittodr() to re-initialize the system time from the RTC hardware (which has presumably been ticking away powered by its own private CR2032). -- Internal to resettodr(), if shutting down, acquiring the internal TODR mutex would be a trylock to avoid getting stuck. I think that's the only serialization needed for the actual RTC hardware, and I think it's reasonable that sleeping should be allowed when accessing it. I would like to add a "shutting_down" global (to go along with "cold") that would automatically transition the i2c layer to polled-operation for the shutdown sequence (to avoid getting stuck writing the RTC ... worst case you just don't get to update it with the kernel's adjusted notion of UTC). Ultimately, I would like to rename inittodr() and resettodr() to something else (todr_copyin() and todr_copyout() maybe? The names today -- init and reset -- aren't really reflective of what they actually do). But I'm not going to die on that hill, so am not proposing any such radical extremist changes at this time :-) Now let's talk about settime1() in kern_time.c. It does a few things that seem dicey vis a vis modifying global state without proper serialization, and just about the entire function is wrapped in an splclock()/splx() pair. First of all, it's not clear to me at all that splclock()/splx() is really needed here... *maybe* across the call to tc_setclock()? But the mutex it takes internally is an IPL_HIGH spin mutex, so splclock() seems superfluous, just a legacy hold-over. ANYWAY... computing "now" and "delta" can be done completely outside a lock. The delta is used to perform authorization and adjust "boottime". The call to tc_setclock() can be done outside a lock. And I think the resettodr() can be done bare, as well (taking into account the TODR synchronization mentioned above). The only thing that requires some synchronization is modifying "boottime", but maybe it's not super critical (all of the consumers of this variable would need significant cleanup. Anyone have any other thoughts on this? -- thorpej
Re: [PATCH] Kernel entropy rework
> Date: Sun, 22 Dec 2019 01:27:58 + > From: > > > On Dec 21, 2019, at 5:08 PM, Taylor R Campbell wrote: > > - For (e.g.) keyboard interrupt and network packet timings, this > >is zero, because an adversary can cause events to happen with > >timing that leads to predictable samples entering the pool. > > That seems overly pessimistic, depending on the timer resolution. > If you have a CPU cycle timer, then it is perfectly reasonable to > claim a bit or two of entropy, since an adversary doesn't have the > ability to control the timing of those events to nanosecond > accuracy, nor the ability to control internal processing delays > (like memory cache misses) which introduce variability way in excess > of a CPU cycle. When I'm typing, there might be nonzero entropy in the cycle counts between keystrokes on my 2.3 GHz laptop. But I don't have a good model, justified by physics, for the probability distribution on the differences in cycle counts. And I definitely don't have one that works for all keyboards on all types of buses on all machines under all typing styles by all adversaries, including my cat sleeping on my keyboard. Do you? We can wave hands about cache misses, but adversaries often have a lot of avenues for influencing the microarchitectural state of the CPU via, e.g., network packets that trigger specific code paths. So I'm not willing to commit to higher entropy estimates for something that was never designed to be unpredictable in the first place unless there's a confidence-inspiring argument -- involving physics and engineering of the hardware -- to justify it. This may well be pessimistic. It would be great if we could make better promises to users -- but only if we can be honest in those promises. Right now we are trading honesty for expediency. My hope is that, for most users, everything else in this change and other recent changes will render moot the motivation for that bargain in the first place. Of course, driver authors have also taken claims by various hardware RNG vendors at face value: - Sometimes they are at least backed up by design documents, like the Intel RDRAND/RDSEED. - Sometimes they are backed up by external audits, like the VIA C3 XSTORE RNG. - Sometimes they are just opaque device registers or DMA commands and you have to do science on them yourself, like the Allwinner Sun8i Crypto Engine TRNG, which seems to have at most 1 bit of min-entropy for every 100 bits of data (and is currently disabled by default until I have a better idea of what to expect from it). What can I honestly promise works? Flip a coin 256 times and run `echo hthhthhhthhh... >> /dev/random'; then the random seed management of rndctl should take care of you for the foreseeable future. Of course, most users don't have the patience for that. Fortunately, most users do not have Intel in their threat model; for those users it is reasonable to rely on Intel RDRAND/RDSEED, for instance.
Re: [PATCH v4 2/4] Fix alignment when reading core notes
In article <20191222164104.165346-2-mgo...@gentoo.org>, MichaŠGórny wrote: >Both desc and note header needs to be aligned. Therefore, we need >to realign after skipping past desc as well. Should probably be done with a standard alignment macro? christos >--- > tests/lib/libc/sys/t_ptrace_wait.c | 3 +++ > 1 file changed, 3 insertions(+) > >diff --git a/tests/lib/libc/sys/t_ptrace_wait.c >b/tests/lib/libc/sys/t_ptrace_wait.c >index 6fc43572e015..1097a351fd92 100644 >--- a/tests/lib/libc/sys/t_ptrace_wait.c >+++ b/tests/lib/libc/sys/t_ptrace_wait.c >@@ -7603,6 +7603,9 @@ static ssize_t core_find_note(const char *core_path, > } > > offset += note_hdr.n_descsz; >+ /* fix to alignment */ >+ offset = ((offset + core_hdr.p_align - 1) >+ / core_hdr.p_align) * core_hdr.p_align; > } > } > >-- >2.24.1 > >
Re: [PATCH] Address USB abort races
> Date: Mon, 23 Dec 2019 14:17:18 + > From: Nick Hudson > > - Any reason that dwc2 and ahci didn't get the conversion? At least dwc2 >should be done - I'm not sure ahci ever worked, but I've tried to keep >it up-to-date in the past. slhci probably needs some love too. I forgot to check it. I searched for *hci but dwc2 doesn't match that pattern! > - It appears to me that upm_abort could be removed in favour of >ubm_abortx and all the xfer cancel logic contained there, but that >could be a future change Root intr transfer handling doesn't go through the same path. Perhaps it could; perhaps that would be simpler. > - Off list you mentioned ohci getting stuck during testing and I would >agree it's unlikely that these diffs caused the issue. I have some >changes to ohci aborting on nhusb which I'll try and fixup when this >diff goes in. Based on some tests I did shortly before leaving for the weekend, I realized it might be a mistake in the root intr transfer handling. Will investigate further.
Re: [PATCH] Address USB abort races
On 17/12/2019 04:34, Taylor R Campbell wrote: The attached change set aims to fix a number of races in the USB transfer complete/abort/timeout protocol by consolidating the logic to synchronize it into a few helper subroutines. (Details below.) Bonus: It is no longer necessary to sleep (other than on an adaptive mutex) when aborting. Thanks for working on this. Some questions/comments if I may - Any reason that dwc2 and ahci didn't get the conversion? At least dwc2 should be done - I'm not sure ahci ever worked, but I've tried to keep it up-to-date in the past. slhci probably needs some love too. - It appears to me that upm_abort could be removed in favour of ubm_abortx and all the xfer cancel logic contained there, but that could be a future change - Off list you mentioned ohci getting stuck during testing and I would agree it's unlikely that these diffs caused the issue. I have some changes to ohci aborting on nhusb which I'll try and fixup when this diff goes in. Thanks, Nick
