| 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
| ||
|
Message-ID: <alpine.LSU.2.20.1708101631080.7971@pobox.suse.cz>
Date: Thu, 10 Aug 2017 16:40:05 +0200 (CEST)
From: Miroslav Benes <mbenes@...e.cz>
To: Joe Lawrence <joe.lawrence@...hat.com>
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
It generally looks ok. Only few questions below...
[...]
> +In-flight parent objects
> +------------------------
> +
> +Sometimes it may not be convenient or possible to allocate shadow
> +variables alongside their parent objects. Or a livepatch fix may
> +require shadow varibles to only a subset of parent object instances. In
> +these cases, the klp_shadow_get_or_attach() call can be used to attach
> +shadow variables to parents already in-flight.
> +
> +For commit 1d147bfa6429, a good spot to attach a shadow spinlock is
> +inside ieee80211_sta_ps_deliver_wakeup():
> +
> +#define PS_LOCK 1
> +void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
> +{
> + DEFINE_SPINLOCK(ps_lock_fallback);
> + spinlock_t *ps_lock;
> +
> + /* sync with ieee80211_tx_h_unicast_ps_buf */
> + 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?
[...]
> +/*
> + * 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.
> +/**
> + * klp_shadow_attach() - allocate and add a new shadow variable
> + * @obj: pointer to parent object
> + * @id: data identifier
> + * @data: pointer to data to attach to parent
> + * @size: size of attached data
> + * @gfp_flags: GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, this routine
> + * will issue a WARN, exit early and return NULL.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space. If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed. The new shadow variable is then added to the
> + * global hashtable.
> + *
> + * Return: the shadow variable data element, NULL on duplicate or
> + * failure.
> + */
> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> + size_t size, gfp_t gfp_flags)
> +{
> + struct klp_shadow *new_shadow;
> + void *shadow_data;
> + unsigned long flags;
> +
> + /* Take error exit path if <obj, id> already exists */
> + shadow_data = klp_shadow_get(obj, id);
> + if (unlikely(shadow_data))
> + goto err_exists;
> +
> + /* Allocate a new shadow variable for use inside the lock below */
> + new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> + if (!new_shadow)
> + goto err;
> +
> + /* Look for <obj, id> again under the lock */
> + spin_lock_irqsave(&klp_shadow_lock, flags);
> + shadow_data = klp_shadow_get(obj, id);
> + if (unlikely(shadow_data)) {
> +
> + /* Shadow variable found, throw away allocation and take
> + * error exit path */
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> + kfree(shadow_data);
> + goto err_exists;
> + }
> +
> + /* No <obj, id> found, add the newly allocated one */
> + shadow_data = data;
> + klp_shadow_set(new_shadow, obj, id, data, size);
> + klp_shadow_add(new_shadow);
> + spin_unlock_irqrestore(&klp_shadow_lock, 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.
> +
> +err_exists:
> + WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +err:
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> +
> +/**
> + * klp_shadow_get_or_attach() - get existing or attach a new shadow variable
> + * @obj: pointer to parent object
> + * @id: data identifier
> + * @data: pointer to data to attach to parent
> + * @size: size of attached data
> + * @gfp_flags: GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, it will be
> + * used (but *not* updated) in the return value of this function.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space. If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed. The new shadow variable is then added to the
> + * global hashtable.
> + *
> + * Return: the shadow variable data element, NULL on failure.
> + */
> +void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> + size_t size, gfp_t gfp_flags)
> +{
> + struct klp_shadow *new_shadow;
> + void *shadow_data;
> + unsigned long flags;
> +
> + /* Return a shadow variable if <obj, id> already exists */
> + shadow_data = klp_shadow_get(obj, id);
> + if (shadow_data)
> + goto ret;
This could be "return ret;" directly. But no big deal.
> + /* Allocate a new shadow variable for use inside the lock below */
> + new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> + if (!new_shadow)
> + goto err;
> +
> + /* Look for <obj, id> again under the lock */
> + spin_lock_irqsave(&klp_shadow_lock, flags);
> + shadow_data = klp_shadow_get(obj, id);
> + if (unlikely(shadow_data)) {
> +
> + /* Shadow variable found, throw away allocation and
> + * return the shadow variable data */
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> + kfree(shadow_data);
> + goto ret;
> + }
> +
> + /* No <obj, id> found, so attach the newly allocated one */
> + shadow_data = data;
Again. "shadow_data = new_shadow->data;"?
> + klp_shadow_set(new_shadow, obj, id, data, size);
> + klp_shadow_add(new_shadow);
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +ret:
> + return shadow_data;
> +err:
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_get_or_attach);
> +
> +/**
> + * klp_shadow_update_or_attach() - update or attach a new shadow variable
> + * @obj: pointer to parent object
> + * @id: data identifier
> + * @data: pointer to data to attach to parent
> + * @size: size of attached data
> + * @gfp_flags: GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, it will be
> + * updated and used in the return value of this function.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space. If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed. The new shadow variable is then added to the
> + * global hashtable.
> + *
> + * Return: the shadow variable data element, NULL on failure.
> + */
> +void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
> + size_t size, gfp_t gfp_flags)
> +{
> + struct klp_shadow *new_shadow;
> + void *shadow_data;
> + unsigned long flags;
> +
> + /* Update/return a shadow variable if <obj, id> already exists */
> + spin_lock_irqsave(&klp_shadow_lock, flags);
> + shadow_data = klp_shadow_get(obj, id);
> + if (shadow_data)
> + goto update_unlock_ret;
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> + /* Allocate a new shadow variable for use inside the lock below */
> + new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> + if (!new_shadow)
> + goto err;
> +
> + /* Look for <obj, id> again under the lock, this time with an
> + * allocation in hand to use if need to use it. */
> + spin_lock_irqsave(&klp_shadow_lock, flags);
> + shadow_data = klp_shadow_get(obj, id);
> + if (unlikely(shadow_data)) {
> +
> + /* Shadow variable found, throw away allocation and
> + * update/return the existing one */
> + kfree(new_shadow);
> + goto update_unlock_ret;
> + }
> +
> + /* Could not find one, so attach the new one */
> + shadow_data = data;
Dtto.
> + klp_shadow_set(new_shadow, obj, id, data, size);
> + klp_shadow_add(new_shadow);
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> + return shadow_data;
> +
> +update_unlock_ret:
> + /* Update already attached shadow variable */
> + new_shadow = container_of(shadow_data, struct klp_shadow, data);
> + klp_shadow_set(new_shadow, obj, id, data, size);
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> + return shadow_data;
> +err:
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_update_or_attach);
The rest is good.
Thanks,
Miroslav
Powered by blists - more mailing lists