2020-11-30 09:33:15

by Cengiz Can

Subject: [PATCH] media: vidtv: fix read after free

`vidtv_channel_si_destroy` function has a call to
`vidtv_psi_pat_table_destroy` that frees Program Association Table.

However it is followed by a loop that iterates over the count of Program
Map Tables. This obviously accesses an invalid memory.

Eliminate this by making a copy of num_pmt before free'ing Program
Association Table and loop on it instead.

Signed-off-by: Cengiz Can <[email protected]>
---
drivers/media/test-drivers/vidtv/vidtv_channel.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/vidtv/vidtv_channel.c b/drivers/media/test-drivers/vidtv/vidtv_channel.c
index 8ad6c0744d36..4af39a8786a7 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_channel.c
+++ b/drivers/media/test-drivers/vidtv/vidtv_channel.c
@@ -503,10 +503,13 @@ int vidtv_channel_si_init(struct vidtv_mux *m)
void vidtv_channel_si_destroy(struct vidtv_mux *m)
{
u32 i;
+ u16 num_pmt;
+
+ num_pmt = m->si.pat->num_pmt;

vidtv_psi_pat_table_destroy(m->si.pat);

- for (i = 0; i < m->si.pat->num_pmt; ++i)
+ for (i = 0; i < num_pmt; ++i)
vidtv_psi_pmt_table_destroy(m->si.pmt_secs[i]);

kfree(m->si.pmt_secs);
--
2.29.2

2020-12-02 12:22:03

by Mauro Carvalho Chehab

Subject: Re: [PATCH] media: vidtv: fix read after free

Hi Cengiz,

Em Mon, 30 Nov 2020 12:28:26 +0300
Cengiz Can <[email protected]> escreveu:

> `vidtv_channel_si_destroy` function has a call to
> `vidtv_psi_pat_table_destroy` that frees Program Association Table.
>
> However it is followed by a loop that iterates over the count of Program
> Map Tables. This obviously accesses an invalid memory.
>
> Eliminate this by making a copy of num_pmt before free'ing Program
> Association Table and loop on it instead.
>
> Signed-off-by: Cengiz Can <[email protected]>

Collin sent me a similar fix. I'm applying his because:
- it was sent first;
- it uses a better approach: it just moves the
vidtv_psi_pat_table_destroy() call to happen after freeing
the PMT tables. So, it doesn't need an aux var.

Regards,
Mauro

> ---
> drivers/media/test-drivers/vidtv/vidtv_channel.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/test-drivers/vidtv/vidtv_channel.c b/drivers/media/test-drivers/vidtv/vidtv_channel.c
> index 8ad6c0744d36..4af39a8786a7 100644
> --- a/drivers/media/test-drivers/vidtv/vidtv_channel.c
> +++ b/drivers/media/test-drivers/vidtv/vidtv_channel.c
> @@ -503,10 +503,13 @@ int vidtv_channel_si_init(struct vidtv_mux *m)
> void vidtv_channel_si_destroy(struct vidtv_mux *m)
> {
> u32 i;
> + u16 num_pmt;
> +
> + num_pmt = m->si.pat->num_pmt;
>
> vidtv_psi_pat_table_destroy(m->si.pat);
>
> - for (i = 0; i < m->si.pat->num_pmt; ++i)
> + for (i = 0; i < num_pmt; ++i)
> vidtv_psi_pmt_table_destroy(m->si.pmt_secs[i]);
>
> kfree(m->si.pmt_secs);



Thanks,
Mauro