On Sat, Mar 20, 2021 at 04:46:48PM +0100, John Wood wrote:
On Wed, Mar 17, 2021 at 09:00:51PM -0700, Kees Cook wrote:
On Sun, Mar 07, 2021 at 12:30:27PM +0100, John Wood wrote:
+/**
- brute_reset_stats() - Reset the statistical data.
- @stats: Statistics to be reset.
- @is_setid: The executable file has the setid flags set.
- Reset the faults and period and set the last crash timestamp to now. This
- way, it is possible to compute the application crash period at the next
- fault. Also, save the credentials of the current task and update the
- bounds_crossed flag based on a previous network activity and the is_setid
- parameter.
- The statistics to be reset cannot be NULL.
- Context: Must be called with interrupts disabled and brute_stats_ptr_lock
and brute_stats::lock held.- */
+static void brute_reset_stats(struct brute_stats *stats, bool is_setid) +{
- const struct cred *cred = current_cred();
- stats->faults = 0;
- stats->jiffies = get_jiffies_64();
- stats->period = 0;
- stats->saved_cred.uid = cred->uid;
- stats->saved_cred.gid = cred->gid;
- stats->saved_cred.suid = cred->suid;
- stats->saved_cred.sgid = cred->sgid;
- stats->saved_cred.euid = cred->euid;
- stats->saved_cred.egid = cred->egid;
- stats->saved_cred.fsuid = cred->fsuid;
- stats->saved_cred.fsgid = cred->fsgid;
- stats->bounds_crossed = stats->network || is_setid;
+}
I would include brute_reset_stats() in the first patch (and add to it as needed). To that end, it can start with a memset(stats, 0, sizeof(*stats));
So, need all the struct fields to be introduced in the initial patch? Even if they are not needed in the initial patch? I'm confused.
No, I meant try to introduce as much infrastructure as possible early in the series. In this case, I was suggesting having introduced brute_reset_stats() at the start, so that in this patch you'd just be adding the new fields to the function. (Instead of both adding new fields and changing the execution pattern.)
+/**
- brute_network() - Target for the socket_sock_rcv_skb hook.
- @sk: Contains the sock (not socket) associated with the incoming sk_buff.
- @skb: Contains the incoming network data.
- A previous step to detect that a network to local boundary has been crossed
- is to detect if there is network activity. To do this, it is only necessary
- to check if there are data packets received from a network device other than
- loopback.
- It's mandatory to disable interrupts before acquiring brute_stats_ptr_lock
- and brute_stats::lock since the task_free hook can be called from an IRQ
- context during the execution of the socket_sock_rcv_skb hook.
- Return: -EFAULT if the current task doesn't have statistical data. Zero
otherwise.- */
+static int brute_network(struct sock *sk, struct sk_buff *skb) +{
- struct brute_stats **stats;
- unsigned long flags;
- if (!skb->dev || (skb->dev->flags & IFF_LOOPBACK))
return 0;
I wonder if you need to also ignore netlink, unix sockets, etc, or does the IFF_LOOPBACK cover those too?
- stats = brute_stats_ptr(current);
Uhh, is "current" valid here? I actually don't know this hook very well.
I think so, but I will try to study it. Thanks for noted this.
I think you might need to track the mapping of task to sock via security_socket_post_create(), security_socket_accept(), and/or security_socket_connect()?
Perhaps just mark it once with security_socket_post_create(), instead of running a hook on every incoming network packet, too?
-Kees
- read_lock_irqsave(&brute_stats_ptr_lock, flags);
- if (!*stats) {
read_unlock_irqrestore(&brute_stats_ptr_lock, flags);return -EFAULT;- }
- spin_lock(&(*stats)->lock);
- (*stats)->network = true;
- spin_unlock(&(*stats)->lock);
- read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
- return 0;
+}
Thanks, John Wood