linux-kernel - Re: [PATCH v3] livepatch: introduce shadow variable API

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:   Thu, 10 Aug 2017 12:04:32 -0400
From:   Joe Lawrence <joe.lawrence@...hat.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jessica Yu <jeyu@...hat.com>, Jiri Kosina <jikos@...nel.org>,
        Petr Mladek <pmladek@...e.com>
Subject: Re: [PATCH v3] livepatch: introduce shadow variable API

On Thu, Aug 10, 2017 at 04:40:05PM +0200, Miroslav Benes wrote:
> 
> It generally looks ok. Only few questions below...
> 
> [...]
> 
> > +In-flight parent objects
> > +------------------------
> > +	ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
> > +			&ps_lock_fallback, sizeof(ps_lock_fallback),
> > +			GFP_ATOMIC);
> > +
> > +	ps_lock = klp_shadow_get(sta, PS_LOCK);
> > +	if (ps_lock)
> > +		spin_lock(ps_lock);
> 
> ps_lock = klp_shadow_get(sta, PS_LOCK); should not be needed, should it?

Correct, I'll remove it.

> [...]
> 
> > +/*
> > + * klp_shadow_set() - initialize a shadow variable
> > + * @shadow:	shadow variable to initialize
> > + * @obj:	pointer to parent object
> > + * @id:		data identifier
> > + * @data:	pointer to data to attach to parent
> > + * @size:	size of attached data
> > + */
> > +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
> > +				  unsigned long id, void *data, size_t size)
> > +{
> > +	shadow->obj = obj;
> > +	shadow->id = id;
> > +
> > +	if (data)
> > +		memcpy(shadow->data, data, size);
> > +}
> > +
> > +/**
> > + * klp_shadow_add() - add a shadow variable to the hashtable
> > + * @shadow:	shadow variable to add
> > + */
> > +static inline void klp_shadow_add(struct klp_shadow *shadow)
> > +{
> > +	hash_add_rcu(klp_shadow_hash, &shadow->node,
> > +		     (unsigned long)shadow->obj);
> > +}
> 
> It would be nice to add a comment that a caller must hold klp_shadow_lock 
> spinlock.

Since shadow_match(), klp_shadow_set(), and klp_shadow_add() are all in
the same boat, I can mention the lock for those functions as well.  BTW,
is there a convention to drop the "klp_" for static, local routines?  I
should be consistent here.
 
> > +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> > +			size_t size, gfp_t gfp_flags)
> > +	return shadow_data;
> 
> I may be missing something, but shouldn't this return new_shadow->data? 
> You return original data here which seems strange.
> 
> > +void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> > +			       size_t size, gfp_t gfp_flags)
> > +	shadow_data = data;
> 
> Again. "shadow_data = new_shadow->data;"?
> 
> > +void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
> > +				  size_t size, gfp_t gfp_flags)
> > +	shadow_data = data;
> 
> Dtto.
> 

Gah!  Thank you for spotting this!  It was leftover from v1 when there
were only pointers (and not data copies) being thrown about.

Thanks,

-- Joe