If the *ppos is non-zero then simple_write_to_buffer() will not
initialize the start of the buf[] buffer.  It doesn't really make sense
to allow non-zero values for *ppos, so check for that at the start and
return -EINVAL.

Fixes: 28edaacb821c ("drm: zynqmp_dp: Add debugfs interface for compliance testing")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
 drivers/gpu/drm/xlnx/zynqmp_dp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 7fb11b0a44f0..847179476041 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1888,6 +1888,9 @@ static ssize_t zynqmp_dp_pattern_write(struct file *file,
 	ssize_t ret;
 	int pattern;
 
+	if (*ppos != 0)
+		return -EINVAL;
+
 	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf,
 				     count);
 	if (ret < 0)
@@ -2028,6 +2031,9 @@ static ssize_t zynqmp_dp_custom_write(struct file *file,
 	ssize_t ret;
 	char buf[sizeof(dp->test.custom)];
 
+	if (*ppos != 0)
+		return -EINVAL;
+
 	ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
 	if (ret < 0)
 		return ret;
-- 
2.53.0

On 5/25/26 03:16, Dan Carpenter wrote:
> If the *ppos is non-zero then simple_write_to_buffer() will not
> initialize the start of the buf[] buffer.  It doesn't really make sense
> to allow non-zero values for *ppos, so check for that at the start and
> return -EINVAL.

non-zero ppos seems to be handled properly by simple_write_to_buffer.

--Sean

> Fixes: 28edaacb821c ("drm: zynqmp_dp: Add debugfs interface for compliance testing")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 7fb11b0a44f0..847179476041 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1888,6 +1888,9 @@ static ssize_t zynqmp_dp_pattern_write(struct file *file,
>   	ssize_t ret;
>   	int pattern;
>   
> +	if (*ppos != 0)
> +		return -EINVAL;
> +
>   	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf,
>   				     count);
>   	if (ret < 0)
> @@ -2028,6 +2031,9 @@ static ssize_t zynqmp_dp_custom_write(struct file *file,
>   	ssize_t ret;
>   	char buf[sizeof(dp->test.custom)];
>   
> +	if (*ppos != 0)
> +		return -EINVAL;
> +
>   	ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
>   	if (ret < 0)
>   		return ret;

On Wed, May 27, 2026 at 08:31:59PM -0400, Sean Anderson wrote:
> On 5/25/26 03:16, Dan Carpenter wrote:
> > If the *ppos is non-zero then simple_write_to_buffer() will not
> > initialize the start of the buf[] buffer.  It doesn't really make sense
> > to allow non-zero values for *ppos, so check for that at the start and
> > return -EINVAL.
> 
> non-zero ppos seems to be handled properly by simple_write_to_buffer.
> 

It's not an overflow bug, it's an uninitialized variable bug.

The simple_write_to_buffer() is designed to handle partial writes so it
leaves the first "written" (scare quotes) part of the string as is.  But
in this case, we can't handle a partial write and the first part of
buf[] is left uninitialized.

https://staticthinking.wordpress.com/2026/05/23/simple_write_to_buffer-is-complicated/

Also this appears to be dead code since fops_zynqmp_dp_pattern is never
used.

regards,
dan carpenter

On 5/28/26 02:18, Dan Carpenter wrote:
> On Wed, May 27, 2026 at 08:31:59PM -0400, Sean Anderson wrote:
>> On 5/25/26 03:16, Dan Carpenter wrote:
>>> If the *ppos is non-zero then simple_write_to_buffer() will not
>>> initialize the start of the buf[] buffer.  It doesn't really make sense
>>> to allow non-zero values for *ppos, so check for that at the start and
>>> return -EINVAL.
>>
>> non-zero ppos seems to be handled properly by simple_write_to_buffer.
>>
> 
> It's not an overflow bug, it's an uninitialized variable bug.
> 
> The simple_write_to_buffer() is designed to handle partial writes so it
> leaves the first "written" (scare quotes) part of the string as is.  But
> in this case, we can't handle a partial write and the first part of
> buf[] is left uninitialized.
> 
> https://staticthinking.wordpress.com/2026/05/23/simple_write_to_buffer-is-complicated/

OK, that's a bit strange. Can you add this to the doc comment? I certainly
missed it when looking around for appropriate functions.

And I like the explicit copy_from_user variant better, especially since
we are setting the nul-terminator anyway.

> Also this appears to be dead code since fops_zynqmp_dp_pattern is never
> used.

It's of course used in zynqmp_dp_bridge_debugfs_init.

--Sean

On Thu, May 28, 2026 at 04:00:53AM -0400, Sean Anderson wrote:
> On 5/28/26 02:18, Dan Carpenter wrote:
> > On Wed, May 27, 2026 at 08:31:59PM -0400, Sean Anderson wrote:
> > > On 5/25/26 03:16, Dan Carpenter wrote:
> > > > If the *ppos is non-zero then simple_write_to_buffer() will not
> > > > initialize the start of the buf[] buffer.  It doesn't really make sense
> > > > to allow non-zero values for *ppos, so check for that at the start and
> > > > return -EINVAL.
> > > 
> > > non-zero ppos seems to be handled properly by simple_write_to_buffer.
> > > 
> > 
> > It's not an overflow bug, it's an uninitialized variable bug.
> > 
> > The simple_write_to_buffer() is designed to handle partial writes so it
> > leaves the first "written" (scare quotes) part of the string as is.  But
> > in this case, we can't handle a partial write and the first part of
> > buf[] is left uninitialized.
> > 
> > https://staticthinking.wordpress.com/2026/05/23/simple_write_to_buffer-is-complicated/
> 
> OK, that's a bit strange. Can you add this to the doc comment? I certainly
> missed it when looking around for appropriate functions.
> 
> And I like the explicit copy_from_user variant better, especially since
> we are setting the nul-terminator anyway.
> 
> > Also this appears to be dead code since fops_zynqmp_dp_pattern is never
> > used.
> 
> It's of course used in zynqmp_dp_bridge_debugfs_init.

Ah, yes.  I just did a git grep and the CREATE_FILE() macro confused me.
But obviously this wouldn't compile if we had an unused local variable.
Duh...

regards,
dan carpenter