Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 22 May 2023 17:36:14 +0200
From:   Michal =?utf-8?Q?Koutn=C3=BD?= <mkoutny@suse.com>
To:     Zheng Wang <zyytlz.wz@163.com>
Cc:     deller@gmx.de, alex000young@gmail.com, linux-fbdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, hackerzheng666@gmail.com,
        dri-devel@lists.freedesktop.org, javierm@redhat.com,
        1395428693sheep@gmail.com, tzimmermann@suse.de
Subject: Re: [PATCH v2] video: imsttfb: Fix use after free bug in
 imsttfb_probe due to lack of error-handling of init_imstt
Message-ID: <34gbv2k3lc5dl4nbivslizfuor6qc34j63idiiuc35qkk3iohs@7bshmqu2ue7a>
References: <20230427030841.2384157-1-zyytlz.wz@163.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="rze7mjqamibzdgst"
Content-Disposition: inline
In-Reply-To: <20230427030841.2384157-1-zyytlz.wz@163.com>
Precedence: bulk


--rze7mjqamibzdgst
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello.

On Thu, Apr 27, 2023 at 11:08:41AM +0800, Zheng Wang <zyytlz.wz@163.com> wr=
ote:
>  static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_i=
d *ent)
> @@ -1529,10 +1530,10 @@ static int imsttfb_probe(struct pci_dev *pdev, co=
nst struct pci_device_id *ent)
>  	if (!par->cmap_regs)
>  		goto error;
>  	info->pseudo_palette =3D par->palette;
> -	init_imstt(info);
> -
> -	pci_set_drvdata(pdev, info);
> -	return 0;
> +	ret =3D init_imstt(info);
> +	if (!ret)
> +		pci_set_drvdata(pdev, info);
> +	return ret;
> =20
>  error:
>  	if (par->dc_regs)

This part caught my eye -- shouldn't the -ENODEV from init_imstt go
through the standard error with proper cleanup? (It seems like a leak
=66rom my 30000 ft view, i.e. not sure about imsttfb_{probe,remove}
pairing.)

Shouldn't there be something like the diff below on top of the existing cod=
e?

Regards,
Michal

diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index 975dd682fae4..a116ac8ca020 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1419,7 +1419,6 @@ static int init_imstt(struct fb_info *info)
 	if ((info->var.xres * info->var.yres) * (info->var.bits_per_pixel >> 3) >=
 info->fix.smem_len
 	    || !(compute_imstt_regvals(par, info->var.xres, info->var.yres))) {
 		printk("imsttfb: %ux%ux%u not supported\n", info->var.xres, info->var.yr=
es, info->var.bits_per_pixel);
-		framebuffer_release(info);
 		return -ENODEV;
 	}
=20
@@ -1455,7 +1454,6 @@ static int init_imstt(struct fb_info *info)
 	fb_alloc_cmap(&info->cmap, 0, 0);
=20
 	if (register_framebuffer(info) < 0) {
-		framebuffer_release(info);
 		return -ENODEV;
 	}
=20
@@ -1531,8 +1529,10 @@ static int imsttfb_probe(struct pci_dev *pdev, const=
 struct pci_device_id *ent)
 		goto error;
 	info->pseudo_palette =3D par->palette;
 	ret =3D init_imstt(info);
-	if (!ret)
-		pci_set_drvdata(pdev, info);
+	if (ret)
+		goto error;
+
+	pci_set_drvdata(pdev, info);
 	return ret;
=20
 error:


--rze7mjqamibzdgst
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTrXXag4J0QvXXBmkMkDQmsBEOquQUCZGuL6wAKCRAkDQmsBEOq
ucOZAP9eACOa+x8XFhfGm7icBBpVmtJLnxaAuj4pG874Tqf53wEAwyYW+IG7SR3D
bfpnLJNb4kEm101zRkgWoaGv6mH4iw0=
=Tk4a
-----END PGP SIGNATURE-----

--rze7mjqamibzdgst--