linux-kernel - RE: [PATCH v1 1/1] IB/hfi1: Don't cast parameter in bit operations

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]

Message-ID: <e39730af26cc4a4d944fa3205fa17b3c@AcuMS.aculab.com>
Date:   Wed, 23 Feb 2022 21:44:32 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Andy Shevchenko' <andriy.shevchenko@...ux.intel.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     Mike Marciniszyn <mike.marciniszyn@...nelisnetworks.com>,
        "Dennis Dalessandro" <dennis.dalessandro@...nelisnetworks.com>
Subject: RE: [PATCH v1 1/1] IB/hfi1: Don't cast parameter in bit operations

From: Andy Shevchenko
> Sent: 23 February 2022 18:54
> 
> While in this particular case it would not be a (critical) issue,
> the pattern itself is bad and error prone in case somebody blindly
> copies to their code.

It is horribly wrong on BE systems.

> Don't cast parameter to unsigned long pointer in the bit operations.
> Instead copy to a local variable on stack of a proper type and use.
> 
> Fixes: 7724105686e7 ("IB/hfi1: add driver files")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/infiniband/hw/hfi1/chip.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/chip.c b/drivers/infiniband/hw/hfi1/chip.c
> index f1245c94ae26..100274b926d3 100644
> --- a/drivers/infiniband/hw/hfi1/chip.c
> +++ b/drivers/infiniband/hw/hfi1/chip.c
> @@ -8286,34 +8286,33 @@ static void is_interrupt(struct hfi1_devdata *dd, unsigned int source)
>  irqreturn_t general_interrupt(int irq, void *data)
>  {
>  	struct hfi1_devdata *dd = data;
> -	u64 regs[CCE_NUM_INT_CSRS];
> +	DECLARE_BITMAP(pending, CCE_NUM_INT_CSRS * 64);
> +	u64 value;
>  	u32 bit;
>  	int i;
> -	irqreturn_t handled = IRQ_NONE;
> 
>  	this_cpu_inc(*dd->int_counter);
> 
>  	/* phase 1: scan and clear all handled interrupts */
>  	for (i = 0; i < CCE_NUM_INT_CSRS; i++) {
> -		if (dd->gi_mask[i] == 0) {
> -			regs[i] = 0;	/* used later */
> -			continue;
> -		}
> -		regs[i] = read_csr(dd, CCE_INT_STATUS + (8 * i)) &
> -				dd->gi_mask[i];
> +		if (dd->gi_mask[i] == 0)
> +			value = 0;	/* used later */
> +		else
> +			value = read_csr(dd, CCE_INT_STATUS + (8 * i)) & dd->gi_mask[i];
> +
> +		/* save for further use */
> +		bitmap_from_u64(&pending[BITS_TO_LONGS(i * 64)], value);
> +
>  		/* only clear if anything is set */
> -		if (regs[i])
> -			write_csr(dd, CCE_INT_CLEAR + (8 * i), regs[i]);
> +		if (value)
> +			write_csr(dd, CCE_INT_CLEAR + (8 * i), value);
>  	}

I think I'd leave all that alone.

>  	/* phase 2: call the appropriate handler */
> -	for_each_set_bit(bit, (unsigned long *)&regs[0],
> -			 CCE_NUM_INT_CSRS * 64) {
> +	for_each_set_bit(bit, pending, CCE_NUM_INT_CSRS * 64)

And do something else for that loop instead.

>  		is_interrupt(dd, bit);
> -		handled = IRQ_HANDLED;
> -	}
> 
> -	return handled;
> +	return IRQ_RETVAL(!bitmap_empty(pending, CCE_NUM_INT_CSRS * 64));

You really don't want to scan the bitmap again.

Actually, of the face of it, you could merge the two loops.
Provided you clear the status bit before calling the relevant
handler I expect it will all work.

	David

>  }
> 
>  irqreturn_t sdma_interrupt(int irq, void *data)
> --
> 2.34.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)