linux-kernel - Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio

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: <87zfia74e6.wl-tiwai@suse.de>
Date: Tue, 25 Feb 2025 10:36:01 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>,
	Lukas Wunner <lukas@...ner.de>,
	Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>,
	Simona Vetter <simona@...ll.ch>,
	Takashi Iwai <tiwai@...e.de>,
	dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org,
	Su Hui <suhui@...china.com>
Subject: Re: [PATCH] vgaswitcheroo: Fix error checking in vga_switcheroo_register_audio_client()

On Tue, 25 Feb 2025 10:10:29 +0100,
Jani Nikula wrote:
> 
> On Mon, 24 Feb 2025, Dan Carpenter <dan.carpenter@...aro.org> wrote:
> > On Mon, Feb 24, 2025 at 03:14:33PM +0200, Jani Nikula wrote:
> >> On Wed, 19 Feb 2025, Dan Carpenter <dan.carpenter@...aro.org> wrote:
> >> > The "id" variable is an enum and in this context it's treated as an
> >> > unsigned int so the error handling can never trigger.  The
> >> > ->get_client_id() functions do not currently return any errors but
> >> > I imagine that if they did, then the intention was to return
> >> > VGA_SWITCHEROO_UNKNOWN_ID on error.  Let's check for both negatives
> >> > and UNKNOWN_ID so we'll catch it either way.
> >> >
> >> > Reported-by: Su Hui <suhui@...china.com>
> >> > Closes: https://lore.kernel.org/all/20231026021056.850680-1-suhui@nfschina.com/
> >> > Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound GPU id")
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> >> > ---
> >> >  drivers/gpu/vga/vga_switcheroo.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> >> > index 18f2c92beff8..216fb208eb31 100644
> >> > --- a/drivers/gpu/vga/vga_switcheroo.c
> >> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> >> > @@ -375,7 +375,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
> >> >  	mutex_lock(&vgasr_mutex);
> >> >  	if (vgasr_priv.active) {
> >> >  		id = vgasr_priv.handler->get_client_id(vga_dev);
> >> > -		if (id < 0) {
> >> > +		if ((int)id < 0 || id == VGA_SWITCHEROO_UNKNOWN_ID) {
> >> 
> >> Maybe we want to do something else here... see [1].
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> [1] https://lore.kernel.org/r/CAHk-=wgg2A_iHNwf_JDjYJF=XHnKVGOjGp50FzVWniA2Z010bw@mail.gmail.com
> >> 
> >
> > I feel like my patch is good enough...  I guess the concern here is that
> > GCC could change enums to something else.  I don't think that's likely as
> > it would break a lot of code.  The other option, which is a good option,
> > is to change the function signature for vgasr_priv.handler->get_client_id()
> > so that we do:
> >
> > 	ret = vgasr_priv.handler->get_client_id(vga_dev, &id);
> > 	if (ret)
> > 		return;
> >
> > That's better code, honestly.  But I can't find motivation enough to do
> > the work.
> 
> The more I look at this, the more bonkers 4aaf448fa975 feels.
> 
> Anyway, I don't think ->get_client_id() hooks should return negative
> error codes, and indeed none of them do. None of them return
> VGA_SWITCHEROO_UNKNOWN_ID either, but that would be a valid return.
> 
> I suggest only checking for id == VGA_SWITCHEROO_UNKNOWN_ID. And doing
> that in all the places that have that check, there are two more, but
> they assign the return value to an int. So the int ret should be changed
> to enum vga_switcheroo_unknown_id id I think.

+1, this sounds like the cleanest way.

> Any chance of finding enough motivation to do that? ;)

For whom? ;)


thanks,

Takashi