Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Tue, 20 Nov 2007, Tetsuo Handa wrote:
> Hello.
>
> Paul Moore wrote:
> > My apologies, I mistakenly read the following if statement in your patch:
> >
> > + if (skb == skb_peek(>sk_receive_queue)) {
> > + __skb_unlink(skb, >sk_receive_queue);
> > + atomic_dec(>users);
> > + }
> >
> > I read the conditional as the following:
> >
> > + if (skb = skb_peek(>sk_receive_queue)) {
> >
> > ... which would have caused the problems I was describing. I'm sorry for
> > all
> > of the confusion/frustration, you patient explanations are correct; I was
> > wrong in this particular case.
> No problem.
>
>
>
> To everyone:
>
> Are there any remaining worries with
> skb_recv_datagram()/socket_post_accept()?
>
> If nobody has objection, I'd like to cut these
> skb_recv_datagram()/socket_post_accept() changes
> and submit to -mm tree.
You should send anything which touches core networking to netdev, too, and
get an ack from one of the core developers there.
--
James Morris <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
Hello.
Paul Moore wrote:
> My apologies, I mistakenly read the following if statement in your patch:
>
> + if (skb == skb_peek(>sk_receive_queue)) {
> + __skb_unlink(skb, >sk_receive_queue);
> + atomic_dec(>users);
> + }
>
> I read the conditional as the following:
>
> + if (skb = skb_peek(>sk_receive_queue)) {
>
> ... which would have caused the problems I was describing. I'm sorry for all
> of the confusion/frustration, you patient explanations are correct; I was
> wrong in this particular case.
No problem.
To everyone:
Are there any remaining worries with skb_recv_datagram()/socket_post_accept()?
If nobody has objection, I'd like to cut these
skb_recv_datagram()/socket_post_accept() changes
and submit to -mm tree.
Regards.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Monday 19 November 2007 9:29:52 am Tetsuo Handa wrote:
> Paul Moore wrote:
> > If that is the case then the second call to
> > skb_peek() will return a different skb then the one you passed to
> > security_post_recv_datagram().
>
> Yes. The second call to skb_peek() might return a different skb than the
> one I passed to security_post_recv_datagram().
My apologies, I mistakenly read the following if statement in your patch:
+ if (skb == skb_peek(>sk_receive_queue)) {
+ __skb_unlink(skb, >sk_receive_queue);
+ atomic_dec(>users);
+ }
I read the conditional as the following:
+ if (skb = skb_peek(>sk_receive_queue)) {
... which would have caused the problems I was describing. I'm sorry for all
of the confusion/frustration, you patient explanations are correct; I was
wrong in this particular case.
--
paul moore
linux security @ hp
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
Hello.
Paul Moore wrote:
> If that is the case then the second call to
> skb_peek() will return a different skb then the one you passed to
> security_post_recv_datagram().
Yes. The second call to skb_peek() might return a different skb than the one
I passed to security_post_recv_datagram().
skb_peek() itself doesn't modify reference count nor queue.
| static inline struct sk_buff *skb_peek(struct sk_buff_head *list_)
| {
| struct sk_buff *list = ((struct sk_buff *)list_)->next;
| if (list == (struct sk_buff *)list_)
| list = NULL;
| return list;
| }
> This is significant because you always throw
> away this second skb without first consulting the LSM via
> security_post_recv_datagram().
I just dequeue skb returned by *first* call to skb_peek().
I'm not handling skb returned by *second* call to skb_peek().
The skb returned by *second* call to skb_peek() is later handled by
somebody else's *first* call to skb_peek().
All skb returned by *first* call to skb_peek() are passed to
security_post_recv_datagram().
Only skb returned by *first* call to skb_peek() can become candidate for that
skb_recv_datagram() call.
No skb returned by *second* call to skb_peek() can become candidate for that
skb_recv_datagram() call,
but can become candidate for subsequent skb_recv_datagram() call if the skb is
returned by
*first* call to skb_peek().
Let's consider with named packets.
Suppose a socket has n packets in it's receive queue.
---
P0 P1 P2 ... Pn
---
Case 1:
Thread T0 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
While T0 is in security_post_recv_datagram(),
another thread T1 picks up P0 without MSG_PEEK flag and then calls
security_post_recv_datagram().
security_post_recv_datagram(P0) from T0 returns -EAGAIN (which means "Don't
deliver this packet to caller."),
T0 checks whether P0 is still in socket queue.
T0 finds that P0 is already dequeued, and just drop reference count of P0.
security_post_recv_datagram(P0) from T1 returns -EAGAIN,
T1 drops reference count of P0.
Case 2:
Thread T0 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
While T0 is in security_post_recv_datagram(),
T1 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
security_post_recv_datagram(P0) from T0 returns -EAGAIN,
T0 checks whether P0 is still in socket queue.
T0 finds that P0 is still in queue, and dequeues P0 and drop reference count
of P0.
security_post_recv_datagram(P0) from T1 returns -EAGAIN,
T1 checks whether P0 is still in socket queue,
T1 finds that P0 is already dequeued, and just drop reference count of P0.
Case 3:
Thread T0 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
While T0 is in security_post_recv_datagram(),
T1 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
security_post_recv_datagram(P0) from T1 returns -EAGAIN,
T1 checks whether P0 is still in socket queue.
T1 finds that P0 is still in queue, and dequeues P0 and drop reference count
of P0.
security_post_recv_datagram(P0) from T0 returns -EAGAIN,
T0 checks whether P0 is still in socket queue,
T0 finds that P0 is already dequeued, and just drop reference count of P0.
In which case did you find racy condition that P0 is passed to userland without
LSM check?
Regards.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Saturday 17 November 2007 11:00:20 pm Tetsuo Handa wrote: > Hello. Hello. > Paul Moore wrote: > > Okay, well if that is the case I think you are going to have another > > problem in that you could end up throwing away skbs that haven't been > > through your security_post_recv_datagram() hook because you _always_ > > throw away the result of the second skb_peek(). Once again, if I'm wrong > > please correct me. > > I didn't understand what's wrong with throwing away the result of > the second skb_peek(). My concern is that you stated earlier that you needed to do the second skb_peek() because the first skb may have been removed from the socket queue while your LSM was making an access decision in security_post_recv_datagram(). If that is the case then the second call to skb_peek() will return a different skb then the one you passed to security_post_recv_datagram(). This is significant because you always throw away this second skb without first consulting the LSM via security_post_recv_datagram(). -- paul moore linux security @ hp - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Saturday 17 November 2007 11:00:20 pm Tetsuo Handa wrote: Hello. Hello. Paul Moore wrote: Okay, well if that is the case I think you are going to have another problem in that you could end up throwing away skbs that haven't been through your security_post_recv_datagram() hook because you _always_ throw away the result of the second skb_peek(). Once again, if I'm wrong please correct me. I didn't understand what's wrong with throwing away the result of the second skb_peek(). My concern is that you stated earlier that you needed to do the second skb_peek() because the first skb may have been removed from the socket queue while your LSM was making an access decision in security_post_recv_datagram(). If that is the case then the second call to skb_peek() will return a different skb then the one you passed to security_post_recv_datagram(). This is significant because you always throw away this second skb without first consulting the LSM via security_post_recv_datagram(). -- paul moore linux security @ hp - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
Hello.
Paul Moore wrote:
If that is the case then the second call to
skb_peek() will return a different skb then the one you passed to
security_post_recv_datagram().
Yes. The second call to skb_peek() might return a different skb than the one
I passed to security_post_recv_datagram().
skb_peek() itself doesn't modify reference count nor queue.
| static inline struct sk_buff *skb_peek(struct sk_buff_head *list_)
| {
| struct sk_buff *list = ((struct sk_buff *)list_)-next;
| if (list == (struct sk_buff *)list_)
| list = NULL;
| return list;
| }
This is significant because you always throw
away this second skb without first consulting the LSM via
security_post_recv_datagram().
I just dequeue skb returned by *first* call to skb_peek().
I'm not handling skb returned by *second* call to skb_peek().
The skb returned by *second* call to skb_peek() is later handled by
somebody else's *first* call to skb_peek().
All skb returned by *first* call to skb_peek() are passed to
security_post_recv_datagram().
Only skb returned by *first* call to skb_peek() can become candidate for that
skb_recv_datagram() call.
No skb returned by *second* call to skb_peek() can become candidate for that
skb_recv_datagram() call,
but can become candidate for subsequent skb_recv_datagram() call if the skb is
returned by
*first* call to skb_peek().
Let's consider with named packets.
Suppose a socket has n packets in it's receive queue.
---
P0 P1 P2 ... Pn
---
Case 1:
Thread T0 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
While T0 is in security_post_recv_datagram(),
another thread T1 picks up P0 without MSG_PEEK flag and then calls
security_post_recv_datagram().
security_post_recv_datagram(P0) from T0 returns -EAGAIN (which means Don't
deliver this packet to caller.),
T0 checks whether P0 is still in socket queue.
T0 finds that P0 is already dequeued, and just drop reference count of P0.
security_post_recv_datagram(P0) from T1 returns -EAGAIN,
T1 drops reference count of P0.
Case 2:
Thread T0 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
While T0 is in security_post_recv_datagram(),
T1 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
security_post_recv_datagram(P0) from T0 returns -EAGAIN,
T0 checks whether P0 is still in socket queue.
T0 finds that P0 is still in queue, and dequeues P0 and drop reference count
of P0.
security_post_recv_datagram(P0) from T1 returns -EAGAIN,
T1 checks whether P0 is still in socket queue,
T1 finds that P0 is already dequeued, and just drop reference count of P0.
Case 3:
Thread T0 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
While T0 is in security_post_recv_datagram(),
T1 picks up P0 with MSG_PEEK flag and then calls
security_post_recv_datagram().
security_post_recv_datagram(P0) from T1 returns -EAGAIN,
T1 checks whether P0 is still in socket queue.
T1 finds that P0 is still in queue, and dequeues P0 and drop reference count
of P0.
security_post_recv_datagram(P0) from T0 returns -EAGAIN,
T0 checks whether P0 is still in socket queue,
T0 finds that P0 is already dequeued, and just drop reference count of P0.
In which case did you find racy condition that P0 is passed to userland without
LSM check?
Regards.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Monday 19 November 2007 9:29:52 am Tetsuo Handa wrote:
Paul Moore wrote:
If that is the case then the second call to
skb_peek() will return a different skb then the one you passed to
security_post_recv_datagram().
Yes. The second call to skb_peek() might return a different skb than the
one I passed to security_post_recv_datagram().
My apologies, I mistakenly read the following if statement in your patch:
+ if (skb == skb_peek(sk-sk_receive_queue)) {
+ __skb_unlink(skb, sk-sk_receive_queue);
+ atomic_dec(skb-users);
+ }
I read the conditional as the following:
+ if (skb = skb_peek(sk-sk_receive_queue)) {
... which would have caused the problems I was describing. I'm sorry for all
of the confusion/frustration, you patient explanations are correct; I was
wrong in this particular case.
--
paul moore
linux security @ hp
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
Hello.
Paul Moore wrote:
My apologies, I mistakenly read the following if statement in your patch:
+ if (skb == skb_peek(sk-sk_receive_queue)) {
+ __skb_unlink(skb, sk-sk_receive_queue);
+ atomic_dec(skb-users);
+ }
I read the conditional as the following:
+ if (skb = skb_peek(sk-sk_receive_queue)) {
... which would have caused the problems I was describing. I'm sorry for all
of the confusion/frustration, you patient explanations are correct; I was
wrong in this particular case.
No problem.
To everyone:
Are there any remaining worries with skb_recv_datagram()/socket_post_accept()?
If nobody has objection, I'd like to cut these
skb_recv_datagram()/socket_post_accept() changes
and submit to -mm tree.
Regards.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Tue, 20 Nov 2007, Tetsuo Handa wrote:
Hello.
Paul Moore wrote:
My apologies, I mistakenly read the following if statement in your patch:
+ if (skb == skb_peek(sk-sk_receive_queue)) {
+ __skb_unlink(skb, sk-sk_receive_queue);
+ atomic_dec(skb-users);
+ }
I read the conditional as the following:
+ if (skb = skb_peek(sk-sk_receive_queue)) {
... which would have caused the problems I was describing. I'm sorry for
all
of the confusion/frustration, you patient explanations are correct; I was
wrong in this particular case.
No problem.
To everyone:
Are there any remaining worries with
skb_recv_datagram()/socket_post_accept()?
If nobody has objection, I'd like to cut these
skb_recv_datagram()/socket_post_accept() changes
and submit to -mm tree.
You should send anything which touches core networking to netdev, too, and
get an ack from one of the core developers there.
--
James Morris [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
Hello.
Paul Moore wrote:
> Okay, well if that is the case I think you are going to have another problem
> in that you could end up throwing away skbs that haven't been through your
> security_post_recv_datagram() hook because you _always_ throw away the result
> of the second skb_peek(). Once again, if I'm wrong please correct me.
I didn't understand what's wrong with throwing away the result of
the second skb_peek(). I'm doing similar things that udp_recvmsg() is doing.
| int udp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
| size_t len, int noblock, int flags, int *addr_len)
| {
| try_again:
| skb = skb_recv_datagram(sk, flags, noblock, );
| if (!skb)
| goto out;
| out_free:
| skb_free_datagram(sk, skb);
| out:
| return err;
|
| csum_copy_err:
| UDP_INC_STATS_BH(UDP_MIB_INERRORS, is_udplite);
|
| skb_kill_datagram(sk, skb, flags);
|
| if (noblock)
| return -EAGAIN;
| goto try_again;
| }
The only difference is that I'm not using skb_kill_datagram()
because skb_kill_datagram() uses spin_lock_bh()
while skb_recv_datagram() needs to use spin_lock_irqsave().
> Where did the 'if (skb) return skb;' code go? Don't you need to do you LSM
> call before you return the skb?
Sorry, I should have explicitly inserted rather than a blank line
like:
| error = security_post_recv_datagram(sk, skb, flags);
| if (error)
| goto force_dequeue;
| } while (!wait_for_packet(sk, err, ));
|
| return NULL;
| force_dequeue:
| /* dequeue if MSG_PEEK is set. */
| no_packet:
| *err = error;
| return NULL;
The below is the updated patch.
Regards.
-
Subject: LSM expansion for TOMOYO Linux.
LSM hooks for sending signal:
* task_kill_unlocked is added in sys_kill
* task_tkill_unlocked is added in sys_tkill
* task_tgkill_unlocked is added in sys_tgkill
LSM hooks for network accept and recv:
* socket_post_accept is modified to return int.
* post_recv_datagram is added in skb_recv_datagram.
You can try TOMOYO Linux without this patch, but in that case, you
can't use access control functionality for restricting signal
transmission and incoming network data.
Signed-off-by: Kentaro Takeda <[EMAIL PROTECTED]>
Signed-off-by: Tetsuo Handa <[EMAIL PROTECTED]>
include/linux/security.h | 74 +++
kernel/signal.c | 17 ++
net/core/datagram.c | 29 --
net/socket.c |7 +++-
security/dummy.c | 32 ++--
security/security.c | 25 ++-
6 files changed, 169 insertions(+), 15 deletions(-)
--- linux-2.6.23.orig/include/linux/security.h 2007-11-17 00:35:44.0
+0900
+++ linux-2.6.23/include/linux/security.h 2007-11-17 00:37:26.0
+0900
@@ -657,6 +657,25 @@ struct request_sock;
* @sig contains the signal value.
* @secid contains the sid of the process where the signal originated
* Return 0 if permission is granted.
+ * @task_kill_unlocked:
+ * Check permission before sending signal @sig to the process of @pid
+ * with sys_kill.
+ * @pid contains the pid of target process.
+ * @sig contains the signal value.
+ * Return 0 if permission is granted.
+ * @task_tkill_unlocked:
+ * Check permission before sending signal @sig to the process of @pid
+ * with sys_tkill.
+ * @pid contains the pid of target process.
+ * @sig contains the signal value.
+ * Return 0 if permission is granted.
+ * @task_tgkill_unlocked:
+ * Check permission before sending signal @sig to the process of @pid
+ * with sys_tgkill.
+ * @tgid contains the thread group id.
+ * @pid contains the pid of target process.
+ * @sig contains the signal value.
+ * Return 0 if permission is granted.
* @task_wait:
* Check permission before allowing a process to reap a child process @p
* and collect its status information.
@@ -778,8 +797,12 @@ struct request_sock;
* @socket_post_accept:
* This hook allows a security module to copy security
* information into the newly created socket's inode.
+ * This hook also allows a security module to filter connections
+ * from unwanted peers.
+ * The connection will be aborted if this hook returns nonzero.
* @sock contains the listening socket structure.
* @newsock contains the newly created server socket for connection.
+ * Return 0 if permission is granted.
* @socket_sendmsg:
* Check permission before transmitting a message to another socket.
* @sock contains the socket structure.
@@ -793,6 +816,12 @@ struct request_sock;
* @size contains the size of message structure.
* @flags contains the operational flags.
* Return 0 if permission is granted.
+ * @post_recv_datagram:
+ *
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Friday 16 November 2007 10:45:32 pm Tetsuo Handa wrote: > Paul Moore wrote: > > I might be missing something here, but why do you need to do a skb_peek() > > again? You already have the skb and the sock, just do the unlink. > > The skb might be already dequeued by other thread while I slept inside > security_post_recv_datagram(). Okay, well if that is the case I think you are going to have another problem in that you could end up throwing away skbs that haven't been through your security_post_recv_datagram() hook because you _always_ throw away the result of the second skb_peek(). Once again, if I'm wrong please correct me. > > Second, why not move the 'no_peek' code to just before 'no_packet'? > > Oh, I didn't notice I can insert here. Now I can also move the rest code > like > > | error = security_post_recv_datagram(sk, skb, flags); > | if (error) > | goto force_dequeue; > | > | } while (!wait_for_packet(sk, err, )); Where did the 'if (skb) return skb;' code go? Don't you need to do you LSM call before you return the skb? -- paul moore linux security @ hp - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Friday 16 November 2007 10:45:32 pm Tetsuo Handa wrote: Paul Moore wrote: I might be missing something here, but why do you need to do a skb_peek() again? You already have the skb and the sock, just do the unlink. The skb might be already dequeued by other thread while I slept inside security_post_recv_datagram(). Okay, well if that is the case I think you are going to have another problem in that you could end up throwing away skbs that haven't been through your security_post_recv_datagram() hook because you _always_ throw away the result of the second skb_peek(). Once again, if I'm wrong please correct me. Second, why not move the 'no_peek' code to just before 'no_packet'? Oh, I didn't notice I can insert here. Now I can also move the rest code like | error = security_post_recv_datagram(sk, skb, flags); | if (error) | goto force_dequeue; | | } while (!wait_for_packet(sk, err, timeo)); Where did the 'if (skb) return skb;' code go? Don't you need to do you LSM call before you return the skb? -- paul moore linux security @ hp - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
Hello.
Paul Moore wrote:
Okay, well if that is the case I think you are going to have another problem
in that you could end up throwing away skbs that haven't been through your
security_post_recv_datagram() hook because you _always_ throw away the result
of the second skb_peek(). Once again, if I'm wrong please correct me.
I didn't understand what's wrong with throwing away the result of
the second skb_peek(). I'm doing similar things that udp_recvmsg() is doing.
| int udp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
| size_t len, int noblock, int flags, int *addr_len)
| {
snipped
| try_again:
| skb = skb_recv_datagram(sk, flags, noblock, err);
| if (!skb)
| goto out;
snipped
| out_free:
| skb_free_datagram(sk, skb);
| out:
| return err;
|
| csum_copy_err:
| UDP_INC_STATS_BH(UDP_MIB_INERRORS, is_udplite);
|
| skb_kill_datagram(sk, skb, flags);
|
| if (noblock)
| return -EAGAIN;
| goto try_again;
| }
The only difference is that I'm not using skb_kill_datagram()
because skb_kill_datagram() uses spin_lock_bh()
while skb_recv_datagram() needs to use spin_lock_irqsave().
Where did the 'if (skb) return skb;' code go? Don't you need to do you LSM
call before you return the skb?
Sorry, I should have explicitly inserted snipped rather than a blank line
like:
| error = security_post_recv_datagram(sk, skb, flags);
| if (error)
| goto force_dequeue;
snipped
| } while (!wait_for_packet(sk, err, timeo));
|
| return NULL;
| force_dequeue:
| /* dequeue if MSG_PEEK is set. */
| no_packet:
| *err = error;
| return NULL;
The below is the updated patch.
Regards.
-
Subject: LSM expansion for TOMOYO Linux.
LSM hooks for sending signal:
* task_kill_unlocked is added in sys_kill
* task_tkill_unlocked is added in sys_tkill
* task_tgkill_unlocked is added in sys_tgkill
LSM hooks for network accept and recv:
* socket_post_accept is modified to return int.
* post_recv_datagram is added in skb_recv_datagram.
You can try TOMOYO Linux without this patch, but in that case, you
can't use access control functionality for restricting signal
transmission and incoming network data.
Signed-off-by: Kentaro Takeda [EMAIL PROTECTED]
Signed-off-by: Tetsuo Handa [EMAIL PROTECTED]
include/linux/security.h | 74 +++
kernel/signal.c | 17 ++
net/core/datagram.c | 29 --
net/socket.c |7 +++-
security/dummy.c | 32 ++--
security/security.c | 25 ++-
6 files changed, 169 insertions(+), 15 deletions(-)
--- linux-2.6.23.orig/include/linux/security.h 2007-11-17 00:35:44.0
+0900
+++ linux-2.6.23/include/linux/security.h 2007-11-17 00:37:26.0
+0900
@@ -657,6 +657,25 @@ struct request_sock;
* @sig contains the signal value.
* @secid contains the sid of the process where the signal originated
* Return 0 if permission is granted.
+ * @task_kill_unlocked:
+ * Check permission before sending signal @sig to the process of @pid
+ * with sys_kill.
+ * @pid contains the pid of target process.
+ * @sig contains the signal value.
+ * Return 0 if permission is granted.
+ * @task_tkill_unlocked:
+ * Check permission before sending signal @sig to the process of @pid
+ * with sys_tkill.
+ * @pid contains the pid of target process.
+ * @sig contains the signal value.
+ * Return 0 if permission is granted.
+ * @task_tgkill_unlocked:
+ * Check permission before sending signal @sig to the process of @pid
+ * with sys_tgkill.
+ * @tgid contains the thread group id.
+ * @pid contains the pid of target process.
+ * @sig contains the signal value.
+ * Return 0 if permission is granted.
* @task_wait:
* Check permission before allowing a process to reap a child process @p
* and collect its status information.
@@ -778,8 +797,12 @@ struct request_sock;
* @socket_post_accept:
* This hook allows a security module to copy security
* information into the newly created socket's inode.
+ * This hook also allows a security module to filter connections
+ * from unwanted peers.
+ * The connection will be aborted if this hook returns nonzero.
* @sock contains the listening socket structure.
* @newsock contains the newly created server socket for connection.
+ * Return 0 if permission is granted.
* @socket_sendmsg:
* Check permission before transmitting a message to another socket.
* @sock contains the socket structure.
@@ -793,6 +816,12 @@ struct request_sock;
* @size contains the size of message structure.
* @flags contains the operational flags.
* Return 0 if permission is granted.
+ *
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
Hello.
Thank you for your feedback.
Paul Moore wrote:
> With this patch the 'cpu_flags' variable will be used in two different
> if-blocks in this function and declared locally within each block. Please
> move the 'cpu_flags' declaration to the top of the function so it only needs
> to be declared once.
I see.
> I might be missing something here, but why do you need to do a skb_peek()
> again? You already have the skb and the sock, just do the unlink.
The skb might be already dequeued by other thread while I slept inside
security_post_recv_datagram().
> Two things. First you can probably just call kfree_skb() instead of
> skb_free_datagram().
So far, there is no difference between skb_free_datagram() and kfree_skb().
| void skb_free_datagram(struct sock *sk, struct sk_buff *skb)
| {
| kfree_skb(skb);
| }
udp_recvmsg() thinks it might not be ok to directly call kfree_skb().
But you and skb_kill_datagram() think it is ok to directly call kfree_skb(),
I will do so.
> Second, why not move the 'no_peek' code to just before 'no_packet'?
Oh, I didn't notice I can insert here. Now I can also move the rest code like
| error = security_post_recv_datagram(sk, skb, flags);
| if (error)
| goto force_dequeue;
| } while (!wait_for_packet(sk, err, ));
|
| return NULL;
| force_dequeue:
| /* dequeue if MSG_PEEK is set. */
| no_packet:
| *err = error;
| return NULL;
to reduce indentation.
Thank you.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Friday 16 November 2007 12:34:57 pm [EMAIL PROTECTED]
wrote:
> LSM hooks for network accept and recv:
>* socket_post_accept is modified to return int.
>* post_recv_datagram is added in skb_recv_datagram.
>
> You can try TOMOYO Linux without this patch, but in that case, you
> can't use access control functionality for restricting signal
> transmission and incoming network data.
As discussed a few times before, I'm still not really excited about adding a
new LSM hook in skb_recv_datagram() when we already have hooks to control
locally consumed network traffic. However, I will admit that these existing
hooks do not allow the LSM to block and query userspace for an access
decision like you are trying to do with TOMOYO. I would prefer not to see
this new LSM hook added but I do not have an alternative solution to your
problem so I can't in good conscience completely object to this patch.
Regardless, I have a few comments which are included below ...
> --- linux-2.6-mm.orig/net/core/datagram.c 2007-10-10 05:31:38.0
> +0900 +++ linux-2.6-mm/net/core/datagram.c2007-11-14 15:15:44.0
> +0900 @@ -55,6 +55,7 @@
> #include
> #include
> #include
> +#include
>
> /*
> * Is a socket 'connection oriented' ?
> @@ -178,6 +179,27 @@ struct sk_buff *skb_recv_datagram(struct
> } else
> skb = skb_dequeue(>sk_receive_queue);
>
> + error = security_post_recv_datagram(sk, skb, flags);
> + if (error) {
> + unsigned long cpu_flags;
With this patch the 'cpu_flags' variable will be used in two different
if-blocks in this function and declared locally within each block. Please
move the 'cpu_flags' declaration to the top of the function so it only needs
to be declared once.
> +
> + if (!(flags & MSG_PEEK))
> + goto no_peek;
> +
> + spin_lock_irqsave(>sk_receive_queue.lock,
> + cpu_flags);
> + if (skb == skb_peek(>sk_receive_queue)) {
I might be missing something here, but why do you need to do a skb_peek()
again? You already have the skb and the sock, just do the unlink.
> + __skb_unlink(skb,
> + >sk_receive_queue);
> + atomic_dec(>users);
> + }
> + spin_unlock_irqrestore(>sk_receive_queue.lock,
> +cpu_flags);
> +no_peek:
> + skb_free_datagram(sk, skb);
> + goto no_packet;
Two things. First you can probably just call kfree_skb() instead of
skb_free_datagram(). Second, why not move the 'no_peek' code to just
before 'no_packet'?
--
paul moore
linux security @ hp
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
On Friday 16 November 2007 12:34:57 pm [EMAIL PROTECTED]
wrote:
LSM hooks for network accept and recv:
* socket_post_accept is modified to return int.
* post_recv_datagram is added in skb_recv_datagram.
You can try TOMOYO Linux without this patch, but in that case, you
can't use access control functionality for restricting signal
transmission and incoming network data.
As discussed a few times before, I'm still not really excited about adding a
new LSM hook in skb_recv_datagram() when we already have hooks to control
locally consumed network traffic. However, I will admit that these existing
hooks do not allow the LSM to block and query userspace for an access
decision like you are trying to do with TOMOYO. I would prefer not to see
this new LSM hook added but I do not have an alternative solution to your
problem so I can't in good conscience completely object to this patch.
Regardless, I have a few comments which are included below ...
--- linux-2.6-mm.orig/net/core/datagram.c 2007-10-10 05:31:38.0
+0900 +++ linux-2.6-mm/net/core/datagram.c2007-11-14 15:15:44.0
+0900 @@ -55,6 +55,7 @@
#include net/checksum.h
#include net/sock.h
#include net/tcp_states.h
+#include linux/security.h
/*
* Is a socket 'connection oriented' ?
@@ -178,6 +179,27 @@ struct sk_buff *skb_recv_datagram(struct
} else
skb = skb_dequeue(sk-sk_receive_queue);
+ error = security_post_recv_datagram(sk, skb, flags);
+ if (error) {
+ unsigned long cpu_flags;
With this patch the 'cpu_flags' variable will be used in two different
if-blocks in this function and declared locally within each block. Please
move the 'cpu_flags' declaration to the top of the function so it only needs
to be declared once.
+
+ if (!(flags MSG_PEEK))
+ goto no_peek;
+
+ spin_lock_irqsave(sk-sk_receive_queue.lock,
+ cpu_flags);
+ if (skb == skb_peek(sk-sk_receive_queue)) {
I might be missing something here, but why do you need to do a skb_peek()
again? You already have the skb and the sock, just do the unlink.
+ __skb_unlink(skb,
+ sk-sk_receive_queue);
+ atomic_dec(skb-users);
+ }
+ spin_unlock_irqrestore(sk-sk_receive_queue.lock,
+cpu_flags);
+no_peek:
+ skb_free_datagram(sk, skb);
+ goto no_packet;
Two things. First you can probably just call kfree_skb() instead of
skb_free_datagram(). Second, why not move the 'no_peek' code to just
before 'no_packet'?
--
paul moore
linux security @ hp
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.
Hello.
Thank you for your feedback.
Paul Moore wrote:
With this patch the 'cpu_flags' variable will be used in two different
if-blocks in this function and declared locally within each block. Please
move the 'cpu_flags' declaration to the top of the function so it only needs
to be declared once.
I see.
I might be missing something here, but why do you need to do a skb_peek()
again? You already have the skb and the sock, just do the unlink.
The skb might be already dequeued by other thread while I slept inside
security_post_recv_datagram().
Two things. First you can probably just call kfree_skb() instead of
skb_free_datagram().
So far, there is no difference between skb_free_datagram() and kfree_skb().
| void skb_free_datagram(struct sock *sk, struct sk_buff *skb)
| {
| kfree_skb(skb);
| }
udp_recvmsg() thinks it might not be ok to directly call kfree_skb().
But you and skb_kill_datagram() think it is ok to directly call kfree_skb(),
I will do so.
Second, why not move the 'no_peek' code to just before 'no_packet'?
Oh, I didn't notice I can insert here. Now I can also move the rest code like
| error = security_post_recv_datagram(sk, skb, flags);
| if (error)
| goto force_dequeue;
| } while (!wait_for_packet(sk, err, timeo));
|
| return NULL;
| force_dequeue:
| /* dequeue if MSG_PEEK is set. */
| no_packet:
| *err = error;
| return NULL;
to reduce indentation.
Thank you.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
