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: <CAJ7bepLOJZLwgm6f+RU=-xb0qPim-7VBi+062EJC5yT5_BmmpA@mail.gmail.com>
Date: Fri, 1 Nov 2024 12:14:54 +0530
From: Advait Dhamorikar <advaitdhamorikar@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Fabrizio Castro <fabrizio.castro.jz@...esas.com>, linux-kernel@...r.kernel.org, 
	skhan@...uxfoundation.org, anupnewsmail@...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))

>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.
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