James Carlson wrote: > Peter Dunlap writes: > >> James Carlson wrote: >> >>> 957-966: this doesn't look right to me. I strongly suggest that you >>> contact the TX team ([EMAIL PROTECTED] or >>> [EMAIL PROTECTED]) to discuss the meaning of this >>> flag. Only MAC-aware applications ought to be setting this, and I >>> don't see where this code handles mandatory access controls at all. >>> >>> >> It's not as bad as it looks -- we are giving up the privilege rather >> than acquiring it. >> > > Giving it up for whom? This is blowing away the NET_MAC_AWARE flag on > the proc_t associated with the current CRED(). I don't know what > thread is running here, and thus I don't know what cred_t you're > actually using (and thus what proc_t gets hit). If it's a kernel > thread, then you're disabling the mac-aware processing for the kernel > itself. If it's a user thread, then you're removing mac-aware > processing for some arbitrary user process, and arbitrarily turning > off a special privilege that the process once held. > > I don't see how manipulating the process flags or privileges from > within a driver (particularly something as common and invisible to > applications as a block storage driver) is a useful thing to do. It > might possibly make some sense if you had your own kernel thread or > special daemon process and were manipulating the flags on that, though > that might beg other questions, but doing it on a thread that belongs > to someone else just can't be right. Can it? > > This almost certainly breaks TX. > > What originally caused me to write a code review comment here was the > phrase "[t]here seem to be some security ramifications." Seems, sir? > Nay, it is; I know not "seems." >
It seemed wrong to take the workaround from the (already code reviewed) ON component without taking the comment with it. Look, this is the exact reason I explicitly sought feedback from networking. It sounds like this is something we need to dig into and understand further. We will remove the workaround and re-test with the current ON bits. Maybe this was a problem with an older version of ON. If it behaves as it did before (badly) we will file a bug and solicit help from the networking team. Is that acceptable? > (Which is another way of saying "you need at least a design level > comment here; if not something more significant." Thanks to Sowmini > for the Hamlet reference reminder. ;-}) > > >> CIFS server actually does the same thing: >> > > As I noted in other cases, I don't really care who or what already > does things like this. > > I think I made it clear in a separate e-mail that we are committed to resolving your review comments. My mention of the CIFS server is not intended to be an excuse nor am I waving it around like a free pass. My point is that if this change breaks something then that something is *already* broken. So we should probably file a fairly high priority bug against CIFS server right? > > >> My understanding when I originally came up with this workaround (for the >> CIFS server code) was that since we were using a kernel thread we got >> the NET_MAC_AWARE privilege whether we wanted it or not. Typically a >> socket thread would come from user space and would not have this >> problem. The only components using sockfs from the kernel are iSCSI >> initiator (which doesn't call soaccept), CIFS server (which uses this >> workaround) and now our code. This is the sort of thing that makes me >> eagerly anticipate the volo project. >> >> I will solicit some input from rampart-dev-team. With the explanation >> above do you think its busted or is it just fishy? >> > > It looks busted to me. > > Thanks, that's what I was looking for. -Peter _______________________________________________ networking-discuss mailing list [email protected]
