Re: [RFC PATCH] asm/generic: introduce if_nospec and nospec_barrier

[Mark Rutland <mark.rutland@arm.com>]

Hi Dan,

Thanks for these examples.

On Thu, Jan 04, 2018 at 03:10:51AM +0000, Williams, Dan J wrote:
> Note, the following is Elena's work, I'm just helping poke the upstream
> discussion along while she's offline.
> 
> Elena audited the static analysis reports down to the following
> locations where userspace provides a value for a conditional branch and
> then later creates a dependent load on that same userspace controlled
> value.
> 
> 'osb()', observable memory barrier, resolves to an lfence on x86. My
> concern with these changes is that it is not clear what content within
> the branch block is of concern. Peter's 'if_nospec' proposal combined
> with Mark's 'nospec_load' would seem to clear that up, from a self
> documenting code perspective, and let archs optionally protect entire
> conditional blocks or individual loads within those blocks.

I'm a little concerned by having to use two helpers that need to be paired. It
means that we have to duplicate the bounds information, and I suspect in
practice that they'll often be paired improperly.

I think that we can avoid those problems if we use nospec_ptr() on its own. It
returns NULL if the pointer would be out-of-bounds, so we can use it in the
if-statement.

On x86, that can contain the bounds checks followed be an OSB / lfence, like
if_nospec(). On arm we can use our architected sequence. In either case,
nospec_ptr() returns NULL if the pointer would be out-of-bounds.

Then, rather than sequences like:

	if_nospec(idx < max) {
		val = nospec_array_load(array, idx, max);
		...
	}

... we could have:

	if ((elem_ptr = nospec_array_ptr(array, idx, max)) {
		val = *elem_ptr;
		...
	}

... which also means we don't have to annotate every single load in the branch
if the element is a structure with multiple fields.

Does that sound workable to you?

From a quick scan, it looks like it would fit all of the example cases below.

Thanks,
Mark.

> Note that these are "a human looked at static analysis reports and
> could not rationalize that these are false positives". Specific domain
> knowledge about these paths may find that some of them are indeed false
> positives.
> 
> The change to m_start in kernel/user_namespace.c is interesting because
> that's an example where the nospec_load() approach by itself would need
> to barrier speculation twice whereas if_nospec can do it once for the
> whole block.
> 
> [ forgive any whitespace damage ]
> 
> 8<---
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 3e7e283a44a8..65175bbe805f 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -821,6 +821,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
>  		}
>  		pin = iterm->id;
>  	} else if (index < selector->bNrInPins) {
> +		osb();
>  		pin = selector->baSourceID[index];
>  		list_for_each_entry(iterm, &chain->entities, chain) {
>  			if (!UVC_ENTITY_IS_ITERM(iterm))
> diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
> index 988c8857d78c..cf267b709af6 100644
> --- a/drivers/net/wireless/ath/carl9170/main.c
> +++ b/drivers/net/wireless/ath/carl9170/main.c
> @@ -1388,6 +1388,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
>  
>  	mutex_lock(&ar->mutex);
>  	if (queue < ar->hw->queues) {
> +		osb();
>  		memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));

>  		ret = carl9170_set_qos(ar);
>  	} else {
> diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
> index ab6d39e12069..f024f1ad81ff 100644
> --- a/drivers/net/wireless/intersil/p54/main.c
> +++ b/drivers/net/wireless/intersil/p54/main.c
> @@ -415,6 +415,7 @@ static int p54_conf_tx(struct ieee80211_hw *dev,
>  
>  	mutex_lock(&priv->conf_mutex);
>  	if (queue < dev->queues) {
> +		osb();
>  		P54_SET_QUEUE(priv->qos_params[queue], params->aifs,
>  			params->cw_min, params->cw_max, params->txop);
>  		ret = p54_set_edcf(priv);
> diff --git a/drivers/net/wireless/st/cw1200/sta.c b/drivers/net/wireless/st/cw1200/sta.c
> index 38678e9a0562..b4a2a7ba04e8 100644
> --- a/drivers/net/wireless/st/cw1200/sta.c
> +++ b/drivers/net/wireless/st/cw1200/sta.c
> @@ -619,6 +619,7 @@ int cw1200_conf_tx(struct ieee80211_hw *dev, struct ieee80211_vif *vif,
>  	mutex_lock(&priv->conf_mutex);
>  
>  	if (queue < dev->queues) {
> +		osb();
>  		old_uapsd_flags = le16_to_cpu(priv->uapsd_info.uapsd_flags);
>  
>  		WSM_TX_QUEUE_SET(&priv->tx_queue_params, queue, 0, 0, 0);
> diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
> index d5da3981cefe..dec8b6e087e3 100644
> --- a/drivers/scsi/qla2xxx/qla_mr.c
> +++ b/drivers/scsi/qla2xxx/qla_mr.c
> @@ -2304,10 +2304,12 @@ qlafx00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
>  	req = ha->req_q_map[que];
>  
>  	/* Validate handle. */
> -	if (handle < req->num_outstanding_cmds)
> +	if (handle < req->num_outstanding_cmds) {
> +		osb();
>  		sp = req->outstanding_cmds[handle];
> -	else
> +	} else {
>  		sp = NULL;
> +	}
>  
>  	if (sp == NULL) {
>  		ql_dbg(ql_dbg_io, vha, 0x3034,
> @@ -2655,10 +2657,12 @@ qlafx00_multistatus_entry(struct scsi_qla_host *vha,
>  		req = ha->req_q_map[que];
>  
>  		/* Validate handle. */
> -		if (handle < req->num_outstanding_cmds)
> +		if (handle < req->num_outstanding_cmds) {
> +			osb();
>  			sp = req->outstanding_cmds[handle];
> -		else
> +		} else {
>  			sp = NULL;
> +		}
>  
>  		if (sp == NULL) {
>  			ql_dbg(ql_dbg_io, vha, 0x3044,
> diff --git a/drivers/thermal/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/int340x_thermal/int340x_thermal_zone.c
> index 145a5c53ff5c..d732b34e120d 100644
> --- a/drivers/thermal/int340x_thermal/int340x_thermal_zone.c
> +++ b/drivers/thermal/int340x_thermal/int340x_thermal_zone.c
> @@ -57,15 +57,16 @@ static int int340x_thermal_get_trip_temp(struct thermal_zone_device *zone,
>  	if (d->override_ops && d->override_ops->get_trip_temp)
>  		return d->override_ops->get_trip_temp(zone, trip, temp);
>  
> -	if (trip < d->aux_trip_nr)
> +	if (trip < d->aux_trip_nr) {
> +		osb();
>  		*temp = d->aux_trips[trip];
> -	else if (trip == d->crt_trip_id)
> +	} else if (trip == d->crt_trip_id) {
>  		*temp = d->crt_temp;
> -	else if (trip == d->psv_trip_id)
> +	} else if (trip == d->psv_trip_id) {
>  		*temp = d->psv_temp;
> -	else if (trip == d->hot_trip_id)
> +	} else if (trip == d->hot_trip_id) {
>  		*temp = d->hot_temp;
> -	else {
> +	} else {
>  		for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) {
>  			if (d->act_trips[i].valid &&
>  			    d->act_trips[i].id == trip) {
> diff --git a/fs/udf/misc.c b/fs/udf/misc.c
> index 401e64cde1be..c5394760d26b 100644
> --- a/fs/udf/misc.c
> +++ b/fs/udf/misc.c
> @@ -104,6 +104,8 @@ struct genericFormat *udf_add_extendedattr(struct inode *inode, uint32_t size,
>  					iinfo->i_lenEAttr) {
>  				uint32_t aal =
>  					le32_to_cpu(eahd->appAttrLocation);
> +
> +				osb();
>  				memmove(&ea[offset - aal + size],
>  					&ea[aal], offset - aal);
>  				offset -= aal;
> @@ -114,6 +116,8 @@ struct genericFormat *udf_add_extendedattr(struct inode *inode, uint32_t size,
>  					iinfo->i_lenEAttr) {
>  				uint32_t ial =
>  					le32_to_cpu(eahd->impAttrLocation);
> +
> +				osb();
>  				memmove(&ea[offset - ial + size],
>  					&ea[ial], offset - ial);
>  				offset -= ial;
> @@ -125,6 +129,8 @@ struct genericFormat *udf_add_extendedattr(struct inode *inode, uint32_t size,
>  					iinfo->i_lenEAttr) {
>  				uint32_t aal =
>  					le32_to_cpu(eahd->appAttrLocation);
> +
> +				osb();
>  				memmove(&ea[offset - aal + size],
>  					&ea[aal], offset - aal);
>  				offset -= aal;
> diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
> index 1c65817673db..dbc12007da51 100644
> --- a/include/linux/fdtable.h
> +++ b/include/linux/fdtable.h
> @@ -82,8 +82,10 @@ static inline struct file *__fcheck_files(struct files_struct *files, unsigned i
>  {
>  	struct fdtable *fdt = rcu_dereference_raw(files->fdt);
>  
> -	if (fd < fdt->max_fds)
> +	if (fd < fdt->max_fds) {
> +		osb();
>  		return rcu_dereference_raw(fdt->fd[fd]);
> +	}
>  	return NULL;
>  }
>  
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 246d4d4ce5c7..aa0be8cef2d4 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -648,15 +648,18 @@ static void *m_start(struct seq_file *seq, loff_t *ppos,
>  {
>  	loff_t pos = *ppos;
>  	unsigned extents = map->nr_extents;
> -	smp_rmb();
>  
> -	if (pos >= extents)
> -		return NULL;
> +	/* paired with smp_wmb in map_write */
> +	smp_rmb();
>  
> -	if (extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> -		return &map->extent[pos];
> +	if (pos < extents) {
> +		osb();
> +		if (extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> +			return &map->extent[pos];
> +		return &map->forward[pos];
> +	}
>  
> -	return &map->forward[pos];
> +	return NULL;
>  }
>  
>  static void *uid_m_start(struct seq_file *seq, loff_t *ppos)
> diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
> index 125c1eab3eaa..56909c8fa025 100644
> --- a/net/ipv4/raw.c
> +++ b/net/ipv4/raw.c
> @@ -476,6 +476,7 @@ static int raw_getfrag(void *from, char *to, int offset, int len, int odd,
>  	if (offset < rfv->hlen) {
>  		int copy = min(rfv->hlen - offset, len);
>  
> +		osb();
>  		if (skb->ip_summed == CHECKSUM_PARTIAL)
>  			memcpy(to, rfv->hdr.c + offset, copy);
>  		else
> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
> index 761a473a07c5..0942784f3f8d 100644
> --- a/net/ipv6/raw.c
> +++ b/net/ipv6/raw.c
> @@ -729,6 +729,7 @@ static int raw6_getfrag(void *from, char *to, int offset, int len, int odd,
>  	if (offset < rfv->hlen) {
>  		int copy = min(rfv->hlen - offset, len);
>  
> +		osb();
>  		if (skb->ip_summed == CHECKSUM_PARTIAL)
>  			memcpy(to, rfv->c + offset, copy);
>  		else
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index 8ca9915befc8..7f83abdea255 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -81,6 +81,8 @@ static struct mpls_route *mpls_route_input_rcu(struct net *net, unsigned index)
>  	if (index < net->mpls.platform_labels) {
>  		struct mpls_route __rcu **platform_label =
>  			rcu_dereference(net->mpls.platform_label);
> +
> +		osb();
>  		rt = rcu_dereference(platform_label[index]);
>  	}
>  	return rt;