linux-kernel - Re: PATCH [1/2] gamecube/wii: graphic quantization registers driver (GQR)

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: <Y4wVwz6kC3ThL2Jg@ZenIV>
Date:   Sun, 4 Dec 2022 03:36:35 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Zopolis0 <creatorsmithmdt@...il.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: PATCH [1/2] gamecube/wii: graphic quantization registers driver
 (GQR)

On Sun, Dec 04, 2022 at 02:06:06PM +1100, Zopolis0 wrote:

> +static u32 gqr_values[8];
> +static struct ctl_table_header *gqr_table_header;
> +
> +#define SPR_GQR0 912
> +#define SPR_GQR1 913
> +#define SPR_GQR2 914
> +#define SPR_GQR3 915
> +#define SPR_GQR4 916
> +#define SPR_GQR5 917
> +#define SPR_GQR6 918
> +#define SPR_GQR7 919
> +
> +#define MFSPR_CASE(i) case (i): (*((u32 *)table->data) = mfspr(SPR_GQR##i))
> +#define MTSPR_CASE(i) case (i): mtspr(SPR_GQR##i, *((u32 *)table->data))
> +
> +static int proc_dogqr(ctl_table *table, int write, struct file *file,
> +       void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> + int r;
> +
> + if (!write) { /* if they are reading, update the variable */
> + switch (table->data - (void *)gqr_values) {

That looks very fishy.  First of all, you clearly have table->data set
(by DECLARE_GQR below) to gqr_values + <interger from 0 to 7>.  Trivial
C quiz: what would the value of
	(void *)(gqr_values + 7) - (void *)gqr_values
be, if gqr_values is declared as an array of unsigned int?

	IOW, the values in that switch are wrong.  If anything,
you want (u32 *)table->data - gqr_values.  What's more, SPR_GQR<n>
is simply 912 + n, innit?  So all the crap with switches and ##
is pointless - that thing should simply be

	unsigned reg = (u32 *)table->data - gqr_values;
	
	if (WARN_ON_ONCE(reg > 7))
		return -EINVAL;

	if (!write)
		gqr_values[reg] = mfspr(SPR_GQR0 + reg);

	r = proc_dointvec(table, write, buffer, lenp, ppos);

	if (!r && write) /* if they are writing, update the reg */
		mtspr(SPR_GQR0 + reg, gqr_values[reg]);

	return r;