linux-kernel - Re: [PATCH v11 26/33] counter: Add character device interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Jun 2021 23:16:50 +0900
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     jic23@...nel.org, linux-stm32@...md-mailman.stormreply.com,
        kernel@...gutronix.de, a.fatoum@...gutronix.de,
        kamel.bouhara@...tlin.com, gwendal@...omium.org,
        alexandre.belloni@...tlin.com, david@...hnology.com,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, syednwaris@...il.com,
        patrick.havelange@...ensium.com, fabrice.gasnier@...com,
        mcoquelin.stm32@...il.com, alexandre.torgue@...com,
        o.rempel@...gutronix.de, jarkko.nikula@...ux.intel.com
Subject: Re: [PATCH v11 26/33] counter: Add character device interface

On Wed, Jun 09, 2021 at 11:59:38AM +0300, Dan Carpenter wrote:
> On Wed, Jun 09, 2021 at 05:28:10PM +0900, William Breathitt Gray wrote:
> > On Wed, Jun 09, 2021 at 11:07:08AM +0300, Dan Carpenter wrote:
> > > On Wed, Jun 09, 2021 at 10:31:29AM +0900, William Breathitt Gray wrote:
> > > > +static int counter_set_event_node(struct counter_device *const counter,
> > > > +				  struct counter_watch *const watch,
> > > > +				  const struct counter_comp_node *const cfg)
> > > > +{
> > > > +	struct counter_event_node *event_node;
> > > > +	struct counter_comp_node *comp_node;
> > > > +
> > > 
> > > The caller should be holding the counter->events_list_lock lock but it's
> > > not.
> > 
> > Hi Dan,
> > 
> > The counter_set_event_node() function doesn't access or modify
> > counter->events_list (it works on counter->next_events_list) so holding
> > the counter->events_list_lock here isn't necessary.
> > 
> 
> There needs to be some sort of locking or this function can race with
> itself.  (Two threads add the same event at exactly the same time).  It
> looks like it can also race with counter_disable_events() leading to a
> use after free.

All right, I'll add in a lock to protect this function so it doesn't
race with itself nor counter_disable_events().

> > > > +	/* Search for event in the list */
> > > > +	list_for_each_entry(event_node, &counter->next_events_list, l)
> > > > +		if (event_node->event == watch->event &&
> > > > +		    event_node->channel == watch->channel)
> > > > +			break;
> > > > +
> > > > +	/* If event is not already in the list */
> > > > +	if (&event_node->l == &counter->next_events_list) {
> > > > +		/* Allocate new event node */
> > > > +		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
> 
> Btw, say we decided that we can add/remove events locklessly, then these
> GFP_ATOMICs can be changed to GFP_KERNEL.

Because I'll be using a lock I'll keep these as GFP_ATOMICs afterall.

Thanks,

William Breathitt Gray

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)