On 02/20, Christian Brauner wrote: > > On Tue, Feb 20, 2024 at 10:02:56AM +0100, Oleg Nesterov wrote: > > > > Ah. IIRC criu uses this hack to restore the pending (arbitrary) signals > > collected at dump time. > > > > I was a bit surprise sys_pidfd_send_signal() allows this hack too, I don't > > I think that we simply mirrored the restrictions in the other system > calls. > > > think that criu uses pidfd at restore time, but I do not know. > > Hm, I just checked and it doesn't use pidfd_send_signal(). It uses > pidfds but only for pid reuse detection for RPC clients. But perhaps something else already uses pidfd_send_signal() with info != NULL or with info->si_code == SI_USER, we can't know. Please see below. > So right now si_code is blocked for >= 0 and for SI_TKILL. If we were to > simply ensure that si_code can't be < 0 then this amounts to effectively > blocking @info from being filled in by userspace at all. Because 0 is a > valid value. I'am afraid I misunderstand you again... 0 == SI_USER is not a valid value when siginfo != NULL. Perhaps we can kill the "task_pid(current) != pid" check and just return EPERM if "kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL", I don't think anobody needs pidfd_send_send_signal() to signal yourself. See below. > + /* Currently unused. */ > + if (info) > + return -EINVAL; Well, to me this looks like the unnecessary restriction... And why? But whatever we do, > - /* Only allow sending arbitrary signals to yourself. */ > - ret = -EPERM; > - if ((task_pid(current) != pid) && > - (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) > - goto err; Can I suggest to fix this check in your tree (add type > PIDTYPE_TGID as we discussed) first, then do other changes on top? This way we can revert the next change(s) if we get regressions reports without re-introducing the security problem. Oleg.
