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;
Message-ID: <069f2f78-01f3-9476-d860-2b695c122649@gmx.de>
Date:   Mon, 22 May 2023 20:02:27 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2] video: imsttfb: Fix use after free bug in
 imsttfb_probe due to lack of error-handling of init_imstt
Content-Language: en-US
To:     =?UTF-8?Q?Michal_Koutn=c3=bd?= <mkoutny@suse.com>,
        Zheng Wang <zyytlz.wz@163.com>
Cc:     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
References: <20230427030841.2384157-1-zyytlz.wz@163.com>
 <34gbv2k3lc5dl4nbivslizfuor6qc34j63idiiuc35qkk3iohs@7bshmqu2ue7a>
From:   Helge Deller <deller@gmx.de>
In-Reply-To: <34gbv2k3lc5dl4nbivslizfuor6qc34j63idiiuc35qkk3iohs@7bshmqu2ue7a>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:u/iktjswmR0=;QcgsMcSYN+1sr1fnL/z4Bk3Ctn5
 9f9SghE3HCO1+MHOIOchWMM4GzgxwChhr0Drb3ZsSMP0Ker/pbbSSXiIkTrFDumbYXegUcOW1
 J46ZMmEzpo0Z1UBqtDGvs3iy7y6XfLZNq4IeIl5WNLh+R83gdMGmCYcLEng5ixVSxukwv2FFQ
 VCsbf/O430+FerLasWH0a+O3ufcBF7DvhMMrSAxg75J9izRDV54DZf6AJ05aDOakd5aBspkWb
 sVDEYNZQUTkkOz/EyP/Gdk9Casuws2/LpOYfWyrwADGSwYnNP6wuqgOsVQv/Hk3BIXnPP7EtN
 i0c1QWm/tSlRSzv1ZfutOJ7PfhxFH9C9uojSM2xsiA1e37s3UqQXfT56zxxgK0hCB5fCLWdFV
 3eDiK5oT1W3cCVQ2uSZwDVh2FHNuMj5qPeZrPFscPSFJypjfCjohoFi/FL+AlXO+3TAW7/Sri
 pdiZa+h+Q24UTBmW/3KmZ/S3NhOcdWpcZT/Zx9RFj5RXkmLQOIRWknlj14C3POpRftz0rQ1Kc
 s9XeBdsLQVmxTNOSU/Ash1t17GhucMmpMm4HUI4mOONAU3CaMRWw3f3ScOYfexojrPg2C/SZ3
 SQS838AT8msUxN+Y3JCtz6vr3rESONTp0PpPfa0HgHF5Ne3DUE2UQkXFOrvB1a+o6DQPQFc47
 PU3PhMh60ygwDe5OB118et7FKXZb+/UDIsVjiJnEYDeK9uWiQpoiu5sO5k6eJWY7iMs36OJFH
 t59wldh+YPiV9FSzXtQMklufhpOxh5RsYVA2CjEy/0PDqpS5kAMFEyLY4CvwOBjscRAroph6i
 RniI9PwpC4JS8ZlmMu66ulrhAyraP1zYRCWLpnv1cfwfk8OAumJ4zPk9+ALULDohE5FbqJMTr
 rG0XxMu6RhxFdeNw/7YSrncx4WbjFc8HVT/msNy++JobkWKLZhWx8wbQztnqZMDhhrBfO7/XJ
 hdbNHA==
Precedence: bulk

Hi Michal,

On 5/22/23 17:36, Michal Koutn=C3=BD wrote:
> On Thu, Apr 27, 2023 at 11:08:41AM +0800, Zheng Wang <zyytlz.wz@163.com>=
 wrote:
>>   static int imsttfb_probe(struct pci_dev *pdev, const struct pci_devic=
e_id *ent)
>> @@ -1529,10 +1530,10 @@ static int imsttfb_probe(struct pci_dev *pdev, =
const 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;
>>
>>   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
> from my 30000 ft view, i.e. not sure about imsttfb_{probe,remove}
> pairing.)

Yes, you seem to be right.

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

Yes, but ....


> 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->va=
r.yres, info->var.bits_per_pixel);
> -		framebuffer_release(info);
>   		return -ENODEV;
>   	}
>
> @@ -1455,7 +1454,6 @@ static int init_imstt(struct fb_info *info)
>   	fb_alloc_cmap(&info->cmap, 0, 0);
>
>   	if (register_framebuffer(info) < 0) {
> -		framebuffer_release(info);

That's ^^^  ok, but I think a
	fb_dealloc_cmap()
is missing here ...

... and in the error path of imsttfb_probe() ....

>   		return -ENODEV;
>   	}
>
> @@ -1531,8 +1529,10 @@ static int imsttfb_probe(struct pci_dev *pdev, co=
nst 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;
>
>   error:

Would you mind sending a proper patch?

Thanks for noticing!
Helge