linux-kernel - Re: [PATCH 1/2] irqchip: irq-mst: Add MStar interrupt controller support

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: <f593f5e395c8558657a3f265b7038ec3@kernel.org>
Date:   Wed, 19 Aug 2020 14:42:48 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Mark-PK Tsai <mark-pk.tsai@...iatek.com>
Cc:     alix.wu@...iatek.com, daniel@...f.com, devicetree@...r.kernel.org,
        jason@...edaemon.net, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        matthias.bgg@...il.com, robh+dt@...nel.org, tglx@...utronix.de,
        yj.chiang@...iatek.com
Subject: Re: [PATCH 1/2] irqchip: irq-mst: Add MStar interrupt controller
 support

On 2020-08-19 14:31, Mark-PK Tsai wrote:
> From: Marc Zyngier <maz@...nel.org>
> 
>> > +
>> > +static int mst_intc_domain_alloc(struct irq_domain *domain, unsigned
>> > int virq,
>> > +				 unsigned int nr_irqs, void *data)
>> > +{
>> > +	int i;
>> > +	irq_hw_number_t hwirq;
>> > +	struct irq_fwspec parent_fwspec, *fwspec = data;
>> > +	struct mst_intc_chip_data *cd = (struct mst_intc_chip_data
>> > *)domain->host_data;
>> 
>> No cast necessary here.
>> 
>> > +
>> > +	/* Not GIC compliant */
>> > +	if (fwspec->param_count != 3)
>> > +		return -EINVAL;
>> > +
>> > +	/* No PPI should point to this domain */
>> > +	if (fwspec->param[0])
>> > +		return -EINVAL;
>> > +
>> > +	if (fwspec->param[1] >= cd->nr_irqs)
>> 
>> This condition is bogus, as it doesn't take into account the nr_irqs
>> parameter.
>> 
> 
> 
> The hwirq number need to be in the irq map range. (property:
> mstar,irqs-map-range)
> If it's not, it must be incorrect configuration.

I agree. And since you are checking whether the configuration is 
correct,
it'd better be completely correct.

> So how about use the condition as following?
> 
> if (hwirq >= cd->nr_irqs)
> 	return -EINVAL;

Again, this says nothing of the validity of (hwirq + nr_irqs - 1)...

> 
>> > +		return -EINVAL;
>> > +
>> > +	hwirq = fwspec->param[1];
>> > +	for (i = 0; i < nr_irqs; i++)
>> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> > +					      &mst_intc_chip,
>> > +					      domain->host_data);

... which you are using here.

         M.
-- 
Jazz is not dead. It just smells funny...