Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <6ee9254b-9481-be0e-1bb3-4d65b461f791@foss.st.com>
Date:   Fri, 22 Apr 2022 17:27:22 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] rpmsg: virtio: fix possible double free in
 rpmsg_virtio_add_ctrl_dev()
Content-Language: en-US
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
To:     Hangyu Hua <hbh25y@gmail.com>, <bjorn.andersson@linaro.org>,
        <mathieu.poirier@linaro.org>
CC:     <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20220418101724.42174-1-hbh25y@gmail.com>
 <64aed5f4-bc6b-b8ea-f599-b2c43e35d9bd@foss.st.com>
In-Reply-To: <64aed5f4-bc6b-b8ea-f599-b2c43e35d9bd@foss.st.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 4/22/22 17:16, Arnaud POULIQUEN wrote:
> Hi Hangyu,
> 
> On 4/18/22 12:17, Hangyu Hua wrote:
>> vch will be free in virtio_rpmsg_release_device() when
>> rpmsg_ctrldev_register_device() fails. There is no need to call
>> kfree() again.
>>
>> Fixes: c486682ae1e2 ("rpmsg: virtio: Register the rpmsg_char device")
>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>> ---
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 603233f0686e..3b7b47f785cf 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -851,7 +851,6 @@ static struct rpmsg_device *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev
>>  
>>  	err = rpmsg_ctrldev_register_device(rpdev_ctrl);
>>  	if (err) {
>> -		kfree(vch);
>>  		return ERR_PTR(err);
>>  	}
>>  
> 
> Good catch! I confirmed by testing the error case. There is a double free.
> 
> That said this highlight a quite more complex issue as
> rpmsg_virtio_del_ctrl_dev[1] and rpmsg_ns_register_device(rpdev_ns)error
> case[2] need also some improvements.
> 
> [1]
> https://elixir.bootlin.com/linux/v5.18-rc3/source/drivers/rpmsg/virtio_rpmsg_bus.c#L861
> [2]https://elixir.bootlin.com/linux/v5.18-rc3/source/drivers/rpmsg/virtio_rpmsg_bus.c#L974
> 
> Please find at the end of my mail a V2 patch that should fix more error
> cases.
> As you initiate the fix, do you want to send the V2 or do you prefer
> that I send the fix?


My apologies, I just saw your second patch[1], so what I have to do is to send a new one to fix
rpmsg_virtio_del_ctrl_dev()

[1]https://patchwork.kernel.org/project/linux-remoteproc/patch/20220418093144.40859-1-hbh25y@gmail.com/

For this one:
 Tested-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

Regards
Arnaud

> 
> Thanks,
> Arnaud
> 
> Subject: [PATCH V2] rpmsg: virtio: fix possible double free in rpmsg_probe()
> 
> the virtio_rpmsg_channel structure will be free in
> virtio_rpmsg_release_device() when the device_register() fails or
> when device_unregister is called.
> There is no need to call kfree() again.
> 
> Fixes: c486682ae1e2 ("rpmsg: virtio: Register the rpmsg_char device")
> 
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 3ede25b1f2e4..a65c8be9b11f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -850,10 +850,8 @@ static struct rpmsg_device
> *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev
>  	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
> 
>  	err = rpmsg_ctrldev_register_device(rpdev_ctrl);
> -	if (err) {
> -		kfree(vch);
> +	if (err)
>  		return ERR_PTR(err);
> -	}
> 
>  	return rpdev_ctrl;
>  }
> @@ -862,7 +860,7 @@ static void rpmsg_virtio_del_ctrl_dev(struct
> rpmsg_device *rpdev_ctrl)
>  {
>  	if (!rpdev_ctrl)
>  		return;
> -	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
> +	device_unregister(&rpdev_ctrl->dev);
>  }
> 
>  static int rpmsg_probe(struct virtio_device *vdev)
> @@ -973,7 +971,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> 
>  		err = rpmsg_ns_register_device(rpdev_ns);
>  		if (err)
> -			goto free_vch;
> +			goto free_ctrldev;
>  	}
> 
>  	/*
> @@ -997,8 +995,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> 
>  	return 0;
> 
> -free_vch:
> -	kfree(vch);
>  free_ctrldev:
>  	rpmsg_virtio_del_ctrl_dev(rpdev_ctrl);
>  free_coherent: