linux-kernel - Re: [RFC PATCH] amd64-agp: do not bind to pci driver if probing fails

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: <7rv3j2it6wbv4gu7jgsews3niste5y52h67scwwjpblhy2royh@hqfmpbjzdj77>
Date: Sat, 21 Jun 2025 19:15:31 +0300
From: Ahmed Salem <x0rw3ll@...il.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: airlied@...hat.com, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, linux-kernel-mentees@...ts.linux.dev
Subject: Re: [RFC PATCH] amd64-agp: do not bind to pci driver if probing fails

Hi Lukas,

On 25/06/21 11:46AM, Lukas Wunner wrote:
> On Sat, Jun 21, 2025 at 04:55:52AM +0300, Ahmed Salem wrote:
> > --- a/drivers/char/agp/amd64-agp.c
> > +++ b/drivers/char/agp/amd64-agp.c
> > @@ -768,10 +768,15 @@ int __init agp_amd64_init(void)
> >  
> >  		/* Look for any AGP bridge */
> >  		agp_amd64_pci_driver.id_table = agp_amd64_pci_promisc_table;
> > -		err = driver_attach(&agp_amd64_pci_driver.driver);
> > -		if (err == 0 && agp_bridges_found == 0) {
> > +		if ((int *)agp_amd64_pci_driver.probe != 0) {
> >  			pci_unregister_driver(&agp_amd64_pci_driver);
> >  			err = -ENODEV;
> > +		} else {
> > +			err = driver_attach(&agp_amd64_pci_driver.driver);
> > +			if (err == 0 && agp_bridges_found == 0) {
> > +				pci_unregister_driver(&agp_amd64_pci_driver);
> > +				err = -ENODEV;
> > +			}
> 
> Is the "probe" member in agp_amd64_pci_driver overwritten with a
> zero pointer anywhere?  I don't see that it is, so it seems the
> else-branch is never entered.
> 

That is a great question. I thought since pci_register_driver calls the
probe function, it would return with or without a zero, saving that
value in the .probe member. (I find my lack of C
experience...disturbing)

> I had already prepared a fix for this, but waited for 0-day to
> crunch through it.  I've just submitted it, so that's what I had
> in mind:
> 
> https://lore.kernel.org/r/f8ff40f35a9a5836d1371f60e85c09c5735e3c5e.1750497201.git.lukas@wunner.de/
> 

That one I've seen even prior to catching this one, and this is
originally what I had in mind based on what commit 6fd024893911
("amd64-agp: Probe unknown AGP devices the right way") removed (i.e.
!pci_find_capability) when you suggested checking for caps beforehand,
but I figured "why make other calls when .probe already does it right
off the bat?" Clearly, I was in the wrong there.

Nonetheless, thank you so much for the review, and Ccing me in the above
patch. Ultimately, what matters to me is getting the right fix in order,
and learning not only what I'd missed here (i.e. Reported-by trailer;
just wasn't quite certain how to approach the situation with SDN in
mind, as well as the original Cced developers), but also fundamental C
knowledge I've been trying to acquire. I appreciate you!

--
Best regards,
Ahmed Salem <x0rw3ll@...il.com>

> Thanks,
> 
> Lukas