Patchwork [Open-FCoE] libfc: sanity check cpu number extracted from xid

login
register
mail settings
Submitter Chris Leech
Date June 29, 2016, 6:11 p.m.
Message ID <1467223878-15950-1-git-send-email-cleech@redhat.com>
Download mbox | patch
Permalink /patch/322/
State New
Headers show

Comments

Chris Leech - June 29, 2016, 6:11 p.m.
In the receive path libfc extracts a cpu number from the ox_id in the
fiber channel header and uses that to do a per_cpu_ptr conversion.
If, for some reason, a frame is received with an invalid ox_id,
per_cpu_ptr will return an invalid pointer and the libfc receive path
will panic the system trying to use it.

I'm currently looking at such a case, and I don't yet know why a
cpu number > nr_cpu_ids is appearing in an exchange id.  But adding a
sanity check in libfc prevents a system panic, and seems like good idea
when dealing with frames coming in from the network.

Signed-off-by: Chris Leech <cleech@redhat.com>
---
 drivers/scsi/libfc/fc_exch.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
Vasu Dev - June 29, 2016, 6:46 p.m.
On Wed, 2016-06-29 at 11:11 -0700, Chris Leech wrote:
> In the receive path libfc extracts a cpu number from the ox_id in the
> fiber channel header and uses that to do a per_cpu_ptr conversion.
> If, for some reason, a frame is received with an invalid ox_id,
> per_cpu_ptr will return an invalid pointer and the libfc receive path
> will panic the system trying to use it.
> 
> I'm currently looking at such a case, and I don't yet know why a
> cpu number > nr_cpu_ids is appearing in an exchange id.  But adding a
> sanity check in libfc prevents a system panic, and seems like good
> idea
> when dealing with frames coming in from the network.
> 
> Signed-off-by: Chris Leech <cleech@redhat.com>
> ---
>  drivers/scsi/libfc/fc_exch.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libfc/fc_exch.c
> b/drivers/scsi/libfc/fc_exch.c
> index 30f9ef0..b33a13c2 100644
> --- a/drivers/scsi/libfc/fc_exch.c
> +++ b/drivers/scsi/libfc/fc_exch.c
> @@ -908,9 +908,16 @@ static struct fc_exch *fc_exch_find(struct
> fc_exch_mgr *mp, u16 xid)
>  {
>  	struct fc_exch_pool *pool;
>  	struct fc_exch *ep = NULL;
> +	u16 cpu = xid & fc_cpu_mask;
> +
> +	if (cpu >= nr_cpu_ids || !cpu_possible(cpu)) {

Good safety checks.

> +		printk(KERN_ERR "libfc: lookup request for XID = %d,
> "
> +				"indicates invalid CPU %d\n", xid,
> cpu);

If it is rare instance then printk is fine, otherwise it would be
unneccessary noise, so better to remove this printk also.


> +		return NULL;
> +	}
>  
>  	if ((xid >= mp->min_xid) && (xid <= mp->max_xid)) {
> -		pool = per_cpu_ptr(mp->pool, xid & fc_cpu_mask);
> +		pool = per_cpu_ptr(mp->pool, cpu);

Still steer to same CPU on which IO originated, looks good.

>  		spin_lock_bh(&pool->lock);
>  		ep = fc_exch_ptr_get(pool, (xid - mp->min_xid) >>
> fc_cpu_order);
>  		if (ep) {

If no noise issue then my pre ACK.

Acked-by : Vasu Dev <vasu.dev@intel.com>

Thanks Chris,
Vasu
Johannes Thumshirn - June 30, 2016, 7:09 a.m.
[ +Cc linux-scsi ]
On Wed, Jun 29, 2016 at 11:46:38AM -0700, Vasu Dev wrote:
> On Wed, 2016-06-29 at 11:11 -0700, Chris Leech wrote:
> > In the receive path libfc extracts a cpu number from the ox_id in the
> > fiber channel header and uses that to do a per_cpu_ptr conversion.
> > If, for some reason, a frame is received with an invalid ox_id,
> > per_cpu_ptr will return an invalid pointer and the libfc receive path
> > will panic the system trying to use it.
> > 
> > I'm currently looking at such a case, and I don't yet know why a
> > cpu number > nr_cpu_ids is appearing in an exchange id.  But adding a
> > sanity check in libfc prevents a system panic, and seems like good
> > idea
> > when dealing with frames coming in from the network.
> > 
> > Signed-off-by: Chris Leech <cleech@redhat.com>
> > ---
> >  drivers/scsi/libfc/fc_exch.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/libfc/fc_exch.c
> > b/drivers/scsi/libfc/fc_exch.c
> > index 30f9ef0..b33a13c2 100644
> > --- a/drivers/scsi/libfc/fc_exch.c
> > +++ b/drivers/scsi/libfc/fc_exch.c
> > @@ -908,9 +908,16 @@ static struct fc_exch *fc_exch_find(struct
> > fc_exch_mgr *mp, u16 xid)
> >  {
> >  	struct fc_exch_pool *pool;
> >  	struct fc_exch *ep = NULL;
> > +	u16 cpu = xid & fc_cpu_mask;
> > +
> > +	if (cpu >= nr_cpu_ids || !cpu_possible(cpu)) {
> 
> Good safety checks.
> 
> > +		printk(KERN_ERR "libfc: lookup request for XID = %d,
> > "
> > +				"indicates invalid CPU %d\n", xid,
> > cpu);
> 
> If it is rare instance then printk is fine, otherwise it would be
> unneccessary noise, so better to remove this printk also.

Can we agree on printk_ratelimited? If it's a rare event we get the info it
happened and if we get a storm of bad frames we won't get DoSed by the log
messages.

> 
> 
> > +		return NULL;
> > +	}
> >  
> >  	if ((xid >= mp->min_xid) && (xid <= mp->max_xid)) {
> > -		pool = per_cpu_ptr(mp->pool, xid & fc_cpu_mask);
> > +		pool = per_cpu_ptr(mp->pool, cpu);
> 
> Still steer to same CPU on which IO originated, looks good.
> 
> >  		spin_lock_bh(&pool->lock);
> >  		ep = fc_exch_ptr_get(pool, (xid - mp->min_xid) >>
> > fc_cpu_order);
> >  		if (ep) {
> 
> If no noise issue then my pre ACK.
> 
> Acked-by : Vasu Dev <vasu.dev@intel.com>
> 
> Thanks Chris,
> Vasu

Other than that,
Acked-by: Johannes Thumshirn <jth@kernel.org>
Chris Leech - June 30, 2016, 3:32 p.m.
On Thu, Jun 30, 2016 at 09:09:25AM +0200, Johannes Thumshirn wrote:
> On Wed, Jun 29, 2016 at 11:46:38AM -0700, Vasu Dev wrote:
> > On Wed, 2016-06-29 at 11:11 -0700, Chris Leech wrote:
> > > +		printk(KERN_ERR "libfc: lookup request for XID = %d,
> > > "
> > > +				"indicates invalid CPU %d\n", xid,
> > > cpu);
> > 
> > If it is rare instance then printk is fine, otherwise it would be
> > unneccessary noise, so better to remove this printk also.
> 
> Can we agree on printk_ratelimited? If it's a rare event we get the info it
> happened and if we get a storm of bad frames we won't get DoSed by the log
> messages.

printk_ratelimited sounds good to me, thanks.

Chris

Patch

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 30f9ef0..b33a13c2 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -908,9 +908,16 @@  static struct fc_exch *fc_exch_find(struct fc_exch_mgr *mp, u16 xid)
 {
 	struct fc_exch_pool *pool;
 	struct fc_exch *ep = NULL;
+	u16 cpu = xid & fc_cpu_mask;
+
+	if (cpu >= nr_cpu_ids || !cpu_possible(cpu)) {
+		printk(KERN_ERR "libfc: lookup request for XID = %d, "
+				"indicates invalid CPU %d\n", xid, cpu);
+		return NULL;
+	}
 
 	if ((xid >= mp->min_xid) && (xid <= mp->max_xid)) {
-		pool = per_cpu_ptr(mp->pool, xid & fc_cpu_mask);
+		pool = per_cpu_ptr(mp->pool, cpu);
 		spin_lock_bh(&pool->lock);
 		ep = fc_exch_ptr_get(pool, (xid - mp->min_xid) >> fc_cpu_order);
 		if (ep) {