On Tue, Feb 15, 2022 at 9:03 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > On Mon, Feb 14, 2022 at 11:13 PM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > Looks okay to me. > > > > The difference from the old one is that: with > > selinux_sctp_process_new_assoc() called in > > selinux_sctp_assoc_established(), the client sksec->peer_sid is using > > the first asoc's peer_secid, instead of the latest asoc's peer_secid. > > And not sure if it will cause any problems when doing the extra check > > sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns > > err*. But I don't know about selinux, I guess there must be a reason > > from selinux side. > > Generally speaking we don't want to change any SELinux socket labels > once it has been created. While the peer_sid is a bit different, > changing it after userspace has access to the socket could be > problematic. In the case where the peer_sid differs between the two > we have a permission check which allows policy to control this > behavior which seems like the best option at this point. I think that maybe Xin was referring to the fact that on error return from the hook the return code information is lost and the assoc is just silently dropped (but I may have misunderstood). In case of a denial (avc_has_perm() returning -EACCESS) this isn't much of a problem, because the denial is logged in the audit log, so there is a way to figure out why opening the association failed. In case of other errors we could indeed do better and either log an SELINUX_ERR audit event or at least pr_err() into the console, but there are likely several other existing cases like this, so it would be best to do this cleanup independently in another patch (if anyone feels up to the task...). -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
