Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Ding Yihan]

Hi Thomas and Abd-Alrhman,

While looking into this exact same syzbot report, I noticed that 
`vidtv_ts_pcr_write_into()` in the same file also suffers from the
 exact same pass-by-value anti-pattern (passing `struct pcr_write_args` by value). 

Since `pcr_write_args` also contains implicit padding, it remains a potential trigger
 for identical KMSAN uninit-value warnings during fuzzing in the future.

Also, regarding Thomas's concern about modifying shared data: passing the struct 
as a `const pointer` (e.g., `const struct null_packet_write_args *`) 
would perfectly guarantee that the state remains read-only.

Thomas, would it be worth submitting a separate patch now to fix `vidtv_ts_pcr_write_into()`
to prevent future KMSAN errors? Or would you prefer this to be addressed together in Abd-Alrhman's v2?

Best regards,
Yihan Ding

Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Thomas Weißschuh]

Hi Yihan Ding,

On Fri, Feb 20, 2026 at 09:39:45PM +0800, Ding Yihan wrote:
> While looking into this exact same syzbot report, I noticed that 
> `vidtv_ts_pcr_write_into()` in the same file also suffers from the
>  exact same pass-by-value anti-pattern (passing `struct pcr_write_args` by value). 

Good catch.
 
> Since `pcr_write_args` also contains implicit padding, it remains a potential trigger
>  for identical KMSAN uninit-value warnings during fuzzing in the future.

The fact that the report is about implicit padding is valuable information.
It should be part of the commit message.

> Also, regarding Thomas's concern about modifying shared data: passing the struct 
> as a `const pointer` (e.g., `const struct null_packet_write_args *`) 
> would perfectly guarantee that the state remains read-only.

Agreed.

> Thomas, would it be worth submitting a separate patch now to fix
> `vidtv_ts_pcr_write_into()` to prevent future KMSAN errors? Or would you
> prefer this to be addressed together in Abd-Alrhman's v2?

Doing it together sounds better. This is not urgent anyways in my opinion.
But on the other hand I am just a random guy whose commit got wrongly blamed
in the original Fixes tag and I don't know anything about this subsystem.


Thomas

Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Abd-Alrhman Masalkhi]

On Fri, Feb 20, 2026 at 14:56 +0100, Thomas Weißschuh wrote:
> Hi Yihan Ding,
>
> On Fri, Feb 20, 2026 at 09:39:45PM +0800, Ding Yihan wrote:
>> While looking into this exact same syzbot report, I noticed that 
>> `vidtv_ts_pcr_write_into()` in the same file also suffers from the
>>  exact same pass-by-value anti-pattern (passing `struct pcr_write_args` by value). 
>
> Good catch.
>  
>> Since `pcr_write_args` also contains implicit padding, it remains a potential trigger
>>  for identical KMSAN uninit-value warnings during fuzzing in the future.
>
> The fact that the report is about implicit padding is valuable information.
> It should be part of the commit message.
>
>> Also, regarding Thomas's concern about modifying shared data: passing the struct 
>> as a `const pointer` (e.g., `const struct null_packet_write_args *`) 
>> would perfectly guarantee that the state remains read-only.
>
> Agreed.
>
>> Thomas, would it be worth submitting a separate patch now to fix
>> `vidtv_ts_pcr_write_into()` to prevent future KMSAN errors? Or would you
>> prefer this to be addressed together in Abd-Alrhman's v2?
>
> Doing it together sounds better. This is not urgent anyways in my opinion.
> But on the other hand I am just a random guy whose commit got wrongly blamed
> in the original Fixes tag and I don't know anything about this subsystem.
>
Sorry, I have misunderstood how the 'Fixes' tag works.

>
> Thomas

-- 
Best Regards,
Abd-Alrhman

Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Thomas Weißschuh]

On Fri, Feb 20, 2026 at 03:58:13PM +0100, Abd-Alrhman Masalkhi wrote:
> On Fri, Feb 20, 2026 at 14:56 +0100, Thomas Weißschuh wrote:

(...)

> >> Thomas, would it be worth submitting a separate patch now to fix
> >> `vidtv_ts_pcr_write_into()` to prevent future KMSAN errors? Or would you
> >> prefer this to be addressed together in Abd-Alrhman's v2?
> >
> > Doing it together sounds better. This is not urgent anyways in my opinion.
> > But on the other hand I am just a random guy whose commit got wrongly blamed
> > in the original Fixes tag and I don't know anything about this subsystem.
> >
> Sorry, I have misunderstood how the 'Fixes' tag works.

No need to be sorry. I didn't want to come across grumpy, but wanted to
express that I am just a bystander and have no authority over this code.


Thomas

Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Abd-Alrhman Masalkhi]

Hi Yihan Ding,

On Fri, Feb 20, 2026 at 21:39 +0800, Ding Yihan wrote:
> Hi Thomas and Abd-Alrhman,
>
> While looking into this exact same syzbot report, I noticed that 
> `vidtv_ts_pcr_write_into()` in the same file also suffers from the
>  exact same pass-by-value anti-pattern (passing `struct pcr_write_args` by value). 
>
> Since `pcr_write_args` also contains implicit padding, it remains a potential trigger
>  for identical KMSAN uninit-value warnings during fuzzing in the future.
>
> Also, regarding Thomas's concern about modifying shared data: passing the struct 
> as a `const pointer` (e.g., `const struct null_packet_write_args *`) 
> would perfectly guarantee that the state remains read-only.
>
> Thomas, would it be worth submitting a separate patch now to fix `vidtv_ts_pcr_write_into()`
> to prevent future KMSAN errors? Or would you prefer this to be addressed together in Abd-Alrhman's v2?
>
> Best regards,
> Yihan Ding
>

Thanks for pointing that out. I agree that vidtv_ts_pcr_write_into()
should be updated in the same way, since it has identical padding issues.

For v2 I’ll incorporate both fixes together and switch both parameters
add the const modifier, as you and Thomas suggested.

Thanks again for the helpful review.

-- 
Best Regards,
Abd-Alrhman

Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Abd-Alrhman Masalkhi]

On Fri, Feb 20, 2026 at 21:39 +0800, Ding Yihan wrote:
> Hi Thomas and Abd-Alrhman,
>
> While looking into this exact same syzbot report, I noticed that 
> `vidtv_ts_pcr_write_into()` in the same file also suffers from the
>  exact same pass-by-value anti-pattern (passing `struct pcr_write_args` by value). 
>
> Since `pcr_write_args` also contains implicit padding, it remains a potential trigger
>  for identical KMSAN uninit-value warnings during fuzzing in the future.
>
> Also, regarding Thomas's concern about modifying shared data: passing the struct 
> as a `const pointer` (e.g., `const struct null_packet_write_args *`) 
> would perfectly guarantee that the state remains read-only.
>
> Thomas, would it be worth submitting a separate patch now to fix `vidtv_ts_pcr_write_into()`
> to prevent future KMSAN errors? Or would you prefer this to be addressed together in Abd-Alrhman's v2?
>
> Best regards,
> Yihan Ding
>

Hi Yihan Ding,

Thanks again for pointing this out earlier.

I've added a fix for vidtv_ts_pcr_write_into() in v2, and I'd like to
include a Suggested-by: tag for you in the patch. I'm still new here,
but my understanding is that Suggested-by is the appropriate tag in
this case.

-- 
Best Regards,
Abd-Alrhman

[PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Abd-Alrhman Masalkhi]

vidtv_ts_null_write_into() takes null_packet_write_args by value,
causing MSAN to report an uninit-value warning on buf_sz inside
the function.

Fix by passing the struct by pointer instead, avoiding the stack copy
entirely.

Reported-by: syzbot+96f901260a0b2d29cd1a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=96f901260a0b2d29cd1a
Fixes: cd7a5651db26 ("alpha: add missing address argument in call to page_table_check_pte_clear()")
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
 drivers/media/test-drivers/vidtv/vidtv_mux.c |  2 +-
 drivers/media/test-drivers/vidtv/vidtv_ts.c  | 18 +++++++++---------
 drivers/media/test-drivers/vidtv/vidtv_ts.h  |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/test-drivers/vidtv/vidtv_mux.c b/drivers/media/test-drivers/vidtv/vidtv_mux.c
index f99878eff7ac..67a580396112 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_mux.c
+++ b/drivers/media/test-drivers/vidtv/vidtv_mux.c
@@ -363,7 +363,7 @@ static u32 vidtv_mux_pad_with_nulls(struct vidtv_mux *m, u32 npkts)
 	args.continuity_counter = &ctx->cc;
 
 	for (i = 0; i < npkts; ++i) {
-		m->mux_buf_offset += vidtv_ts_null_write_into(args);
+		m->mux_buf_offset += vidtv_ts_null_write_into(&args);
 		args.dest_offset  = m->mux_buf_offset;
 	}
 
diff --git a/drivers/media/test-drivers/vidtv/vidtv_ts.c b/drivers/media/test-drivers/vidtv/vidtv_ts.c
index ca4bb9c40b78..7e6e92503fb8 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_ts.c
+++ b/drivers/media/test-drivers/vidtv/vidtv_ts.c
@@ -48,7 +48,7 @@ void vidtv_ts_inc_cc(u8 *continuity_counter)
 		*continuity_counter = 0;
 }
 
-u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
+u32 vidtv_ts_null_write_into(struct null_packet_write_args *args)
 {
 	u32 nbytes = 0;
 	struct vidtv_mpeg_ts ts_header = {};
@@ -56,21 +56,21 @@ u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
 	ts_header.sync_byte          = TS_SYNC_BYTE;
 	ts_header.bitfield           = cpu_to_be16(TS_NULL_PACKET_PID);
 	ts_header.payload            = 1;
-	ts_header.continuity_counter = *args.continuity_counter;
+	ts_header.continuity_counter = *args->continuity_counter;
 
 	/* copy TS header */
-	nbytes += vidtv_memcpy(args.dest_buf,
-			       args.dest_offset + nbytes,
-			       args.buf_sz,
+	nbytes += vidtv_memcpy(args->dest_buf,
+			       args->dest_offset + nbytes,
+			       args->buf_sz,
 			       &ts_header,
 			       sizeof(ts_header));
 
-	vidtv_ts_inc_cc(args.continuity_counter);
+	vidtv_ts_inc_cc(args->continuity_counter);
 
 	/* fill the rest with empty data */
-	nbytes += vidtv_memset(args.dest_buf,
-			       args.dest_offset + nbytes,
-			       args.buf_sz,
+	nbytes += vidtv_memset(args->dest_buf,
+			       args->dest_offset + nbytes,
+			       args->buf_sz,
 			       TS_FILL_BYTE,
 			       TS_PACKET_LEN - nbytes);
 
diff --git a/drivers/media/test-drivers/vidtv/vidtv_ts.h b/drivers/media/test-drivers/vidtv/vidtv_ts.h
index 09b4ffd02829..28da15dcc697 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_ts.h
+++ b/drivers/media/test-drivers/vidtv/vidtv_ts.h
@@ -90,7 +90,7 @@ void vidtv_ts_inc_cc(u8 *continuity_counter);
  *
  * Return: The number of bytes written into the buffer.
  */
-u32 vidtv_ts_null_write_into(struct null_packet_write_args args);
+u32 vidtv_ts_null_write_into(struct null_packet_write_args *args);
 
 /**
  * vidtv_ts_pcr_write_into - Write a PCR  packet into a buffer.
-- 
2.43.0

Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Thomas Weißschuh]

Hi Abd-Alrhman!

On Mon, Feb 16, 2026 at 10:17:03PM +0100, Abd-Alrhman Masalkhi wrote:
> vidtv_ts_null_write_into() takes null_packet_write_args by value,
> causing MSAN to report an uninit-value warning on buf_sz inside
> the function.
> 
> Fix by passing the struct by pointer instead, avoiding the stack copy
> entirely.
> 
> Reported-by: syzbot+96f901260a0b2d29cd1a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=96f901260a0b2d29cd1a
> Fixes: cd7a5651db26 ("alpha: add missing address argument in call to page_table_check_pte_clear()")

The Fixes tag should point to the commit which originally introduced
the issue that is being fixed. My commit for alpha-specific code is
unlikely to be the root cause because a) syscaller is not running alpha
systems and b) the first occurrence of the syscaller was before my patch
was picked up by Linus. So please try to find a better Fixes tag.

> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
>  drivers/media/test-drivers/vidtv/vidtv_mux.c |  2 +-
>  drivers/media/test-drivers/vidtv/vidtv_ts.c  | 18 +++++++++---------
>  drivers/media/test-drivers/vidtv/vidtv_ts.h  |  2 +-
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vidtv/vidtv_mux.c b/drivers/media/test-drivers/vidtv/vidtv_mux.c
> index f99878eff7ac..67a580396112 100644
> --- a/drivers/media/test-drivers/vidtv/vidtv_mux.c
> +++ b/drivers/media/test-drivers/vidtv/vidtv_mux.c
> @@ -363,7 +363,7 @@ static u32 vidtv_mux_pad_with_nulls(struct vidtv_mux *m, u32 npkts)
>  	args.continuity_counter = &ctx->cc;
>  
>  	for (i = 0; i < npkts; ++i) {
> -		m->mux_buf_offset += vidtv_ts_null_write_into(args);
> +		m->mux_buf_offset += vidtv_ts_null_write_into(&args);

Please explain in the commit message why this is change is safe to do.
Modifying shared data might break the logic. (I have not verified this)

>  		args.dest_offset  = m->mux_buf_offset;
>  	}

(...)

Did have syscaller test this patch before submission?
See the following documentation from the report:

	If you want syzbot to run the reproducer, reply with:
	#syz test: git://repo/address.git branch-or-commit-hash
	If you attach or paste a git patch, syzbot will apply it before testing.


Thomas

Re: [PATCH] media: vidtv: fix uninitialized args.buf_sz passed by value

[Abd-Alrhman Masalkhi]

On Wed, Feb 18, 2026 at 14:24 +0100, Thomas Weißschuh wrote:

Hi Thomas,

Thank you for the feedback.

> Hi Abd-Alrhman!
>
> On Mon, Feb 16, 2026 at 10:17:03PM +0100, Abd-Alrhman Masalkhi wrote:
>> vidtv_ts_null_write_into() takes null_packet_write_args by value,
>> causing MSAN to report an uninit-value warning on buf_sz inside
>> the function.
>> 
>> Fix by passing the struct by pointer instead, avoiding the stack copy
>> entirely.
>> 
>> Reported-by: syzbot+96f901260a0b2d29cd1a@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=96f901260a0b2d29cd1a
>> Fixes: cd7a5651db26 ("alpha: add missing address argument in call to page_table_check_pte_clear()")
>
> The Fixes tag should point to the commit which originally introduced
> the issue that is being fixed. My commit for alpha-specific code is
> unlikely to be the root cause because a) syscaller is not running alpha
> systems and b) the first occurrence of the syscaller was before my patch
> was picked up by Linus. So please try to find a better Fixes tag.
>
I will fix the Fixes tag to point to the correct vidtv commit.

>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>>  drivers/media/test-drivers/vidtv/vidtv_mux.c |  2 +-
>>  drivers/media/test-drivers/vidtv/vidtv_ts.c  | 18 +++++++++---------
>>  drivers/media/test-drivers/vidtv/vidtv_ts.h  |  2 +-
>>  3 files changed, 11 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/media/test-drivers/vidtv/vidtv_mux.c b/drivers/media/test-drivers/vidtv/vidtv_mux.c
>> index f99878eff7ac..67a580396112 100644
>> --- a/drivers/media/test-drivers/vidtv/vidtv_mux.c
>> +++ b/drivers/media/test-drivers/vidtv/vidtv_mux.c
>> @@ -363,7 +363,7 @@ static u32 vidtv_mux_pad_with_nulls(struct vidtv_mux *m, u32 npkts)
>>  	args.continuity_counter = &ctx->cc;
>>  
>>  	for (i = 0; i < npkts; ++i) {
>> -		m->mux_buf_offset += vidtv_ts_null_write_into(args);
>> +		m->mux_buf_offset += vidtv_ts_null_write_into(&args);
>
> Please explain in the commit message why this is change is safe to do.
> Modifying shared data might break the logic. (I have not verified this)
>
vidtv_ts_null_write_into() only reads from the struct and does not
modify it. should I add a const qualifier to the parameter? 

>>  		args.dest_offset  = m->mux_buf_offset;
>>  	}
>
> (...)
>
> Did have syscaller test this patch before submission?
> See the following documentation from the report:
>
> 	If you want syzbot to run the reproducer, reply with:
> 	#syz test: git://repo/address.git branch-or-commit-hash
> 	If you attach or paste a git patch, syzbot will apply it before testing.
>
I will test with syzbot before sending v2.

>
> Thomas

-- 
Best Regards,
Abd-Alrhman