linux-kernel - RE: [PATCH-next] irqchip/renesas-rzv2h: Fix potentially mismatched datatype

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:
 <TY3PR01MB120896A09E7CF11D55164527BC2562@TY3PR01MB12089.jpnprd01.prod.outlook.com>
Date: Fri, 1 Nov 2024 09:48:04 +0000
From: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
To: Advait Dhamorikar <advaitdhamorikar@...il.com>, Thomas Gleixner
	<tglx@...utronix.de>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"skhan@...uxfoundation.org" <skhan@...uxfoundation.org>,
	"anupnewsmail@...il.com" <anupnewsmail@...il.com>
Subject: RE: [PATCH-next] irqchip/renesas-rzv2h: Fix potentially mismatched
 datatype

Hello Advait,

Thanks for your email.

> From: Advait Dhamorikar <advaitdhamorikar@...il.com>
> Subject: Re: [PATCH-next] irqchip/renesas-rzv2h: Fix potentially mismatched datatype
> 
> Hello Thomas,
> 
> > and read through the matching documentation.
> My bad, I will be more imperative next time :)
> 
> > In fact there is no problem with the existing code because the hardware
> > interrupt number range for this interrupt chip is guaranteed to be
> > smaller than UINT_MAX. IOW, a truncation from unsigned long to unsigned
> > int (on a 64-bit system) does not matter at all.
> I did not know about the interrupt range of the chip, so I
> assumed the truncation from 8 bytes to 4 might pose a problem.
> 
> >If at all, then the proper change is either
> >1) to make the related variables type irq_hw_number_t
> This seems like the better option to me. If it is needed,
> I will submit a patch v2 after waiting for some more feedback, if there's any.
> 
> I have one question, static analyzers report an issue of a bad bit
> shift operation
> on line 307: tien = ICU_TSSR_TIEN(titsel_n);
> #define ICU_TSSR_TIEN(n) (BIT(7) << ((n) * 8))

Which static analyzers did you use?

> 
> From what I understand hwirq can possibly have values from 0 to 31
> If titsel_n ends up being a large remainder say 5, we can have a bad
> bitshift operation
> exceeding 64 bits.

In this case it's not a bad bit shift operation. The code never passes a parameter that makes it exceed the 64 bit boundary.

There is nothing to fix in this case.

Remember that analysers are not always right, you still need to read and understand the code.

Kind regards,
Fab

> My humble apologies if my observations are completely off, I'm a
> beginner trying to learn
> Linux driver dev by looking at how other drivers work.
> If this is an issue what could be a possible method to fix this?
> I would be grateful if you or someone could point me to some relevant docs.
> 
> Thank you for your time and feedback,
> 
> Best regards,
> Advait
> 
> On Fri, 1 Nov 2024 at 02:54, Thomas Gleixner <tglx@...utronix.de> wrote:
> >
> > On Fri, Nov 01 2024 at 01:06, Advait Dhamorikar wrote:
> > > This patch updates the type of hw_irq to unsigned long to
> >
> > Please do:
> >
> > git grep 'This patch' Documentation/process/
> >
> > and read through the matching documentation.
> >
> > > match irq_hw_number_t.
> > >
> > > The variable hw_irq is defined as unsigned int at places,
> > > However when it is initialized using irqd_to_hwirq(), it returns
> > > an irq_hw_number_t, which inturn is a typedef for unsigned long.
> >
> > We know that, but what is the problem this patch is actually solving?
> >
> > >  static void rzv2h_icu_eoi(struct irq_data *d)
> > >  {
> > >       struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> > > -     unsigned int hw_irq = irqd_to_hwirq(d);
> > > +     unsigned long hw_irq = irqd_to_hwirq(d);
> > >       unsigned int tintirq_nr;
> >
> > It moves the type mismatch and potential truncation a few lines further
> > down:
> >
> >         tintirq_nr = hw_irq - ICU_TINT_START;
> >
> > In fact there is no problem with the existing code because the hardware
> > interrupt number range for this interrupt chip is guaranteed to be
> > smaller than UINT_MAX. IOW, a truncation from unsigned long to unsigned
> > int (on a 64-bit system) does not matter at all.
> >
> > I'm all for being type safe, but what you are doing is purely cosmetic.
> >
> > If at all, then the proper change is either
> >
> >  1) to make the related variables type irq_hw_number_t
> >
> >     You cannot make assumptions about the type which is behind
> >     irq_hw_number_t today. The type can change tomorrow, no?
> >
> > or
> >
> >  2) Use a proper type cast which documents that the type conversion
> >     including the potential truncation is intentional and correct.
> >
> >     This should not be an actual type cast, but a helper inline which
> >     has the cast and explicitely returns an unsigned int.
> >
> > I leave it to you to decide which variant is the correct one, but I'm
> > happy to answer your questions.
> >
> > Thanks,
> >
> >         tglx