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
--
Kees Cook
