soundwire: sysfs_emit() expects a buffer of size PAGE_SIZE

[PATCH] soundwire: sysfs_emit() expects a buffer of size PAGE_SIZE

Posted by Sergio Perez Gonzalez 2 months, 2 weeks ago

In read_buffer_show(), allocate sufficient memory to pass on to
sysfs_emit(), which expects a buffer of size PAGE_SIZE.

Link: https://scan7.scan.coverity.com/#/project-view/53936/11354?selectedIssue=1648019
Fixes: 35323d8ab811 ("soundwire: replace scnprintf() with sysfs_emit() for sysfs consistency")
Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com>
---
 drivers/soundwire/debugfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index 825f7abbad32..08fb1e29026e 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -306,12 +306,12 @@ static int cmd_go(void *data, u64 value)
 DEFINE_DEBUGFS_ATTRIBUTE(cmd_go_fops, NULL,
 			 cmd_go, "%llu\n");
 
-#define MAX_LINE_LEN 128
-
 static int read_buffer_show(struct seq_file *s_file, void *data)
 {
-	char buf[MAX_LINE_LEN];
 	int i;
+	char *buf __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 
 	if (num_bytes == 0 || num_bytes > MAX_CMD_BYTES)
 		return -EINVAL;
-- 
2.43.0

Re: [PATCH] soundwire: sysfs_emit() expects a buffer of size PAGE_SIZE

Posted by Takashi Iwai 2 months, 1 week ago

On Thu, 24 Jul 2025 01:09:25 +0200,
Sergio Perez Gonzalez wrote:
> 
> In read_buffer_show(), allocate sufficient memory to pass on to
> sysfs_emit(), which expects a buffer of size PAGE_SIZE.
> 
> Link: https://scan7.scan.coverity.com/#/project-view/53936/11354?selectedIssue=1648019
> Fixes: 35323d8ab811 ("soundwire: replace scnprintf() with sysfs_emit() for sysfs consistency")
> Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com>
> ---
>  drivers/soundwire/debugfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index 825f7abbad32..08fb1e29026e 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -306,12 +306,12 @@ static int cmd_go(void *data, u64 value)
>  DEFINE_DEBUGFS_ATTRIBUTE(cmd_go_fops, NULL,
>  			 cmd_go, "%llu\n");
>  
> -#define MAX_LINE_LEN 128
> -
>  static int read_buffer_show(struct seq_file *s_file, void *data)
>  {
> -	char buf[MAX_LINE_LEN];
>  	int i;
> +	char *buf __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;

IMO, it's better to scratch the whole previous change.
It makes little sense to allocate a large buffer here just for the
temporary formatting.

Moreover, *_show() functions there take seq_file pointer, and you can
just use seq_printf() directly.  The sysfs/kobject show callback is
with the a fixed PAGE_SIZE buffer, hence sysfs_emit() is useful, but
in this case, it's a completely different story.


thanks,

Takashi

Re: [PATCH] soundwire: sysfs_emit() expects a buffer of size PAGE_SIZE

Posted by Vinod Koul 2 months, 1 week ago

On 24-07-25, 09:19, Takashi Iwai wrote:
> On Thu, 24 Jul 2025 01:09:25 +0200,
> Sergio Perez Gonzalez wrote:
> > 
> > In read_buffer_show(), allocate sufficient memory to pass on to
> > sysfs_emit(), which expects a buffer of size PAGE_SIZE.
> > 
> > Link: https://scan7.scan.coverity.com/#/project-view/53936/11354?selectedIssue=1648019
> > Fixes: 35323d8ab811 ("soundwire: replace scnprintf() with sysfs_emit() for sysfs consistency")
> > Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com>
> > ---
> >  drivers/soundwire/debugfs.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> > index 825f7abbad32..08fb1e29026e 100644
> > --- a/drivers/soundwire/debugfs.c
> > +++ b/drivers/soundwire/debugfs.c
> > @@ -306,12 +306,12 @@ static int cmd_go(void *data, u64 value)
> >  DEFINE_DEBUGFS_ATTRIBUTE(cmd_go_fops, NULL,
> >  			 cmd_go, "%llu\n");
> >  
> > -#define MAX_LINE_LEN 128
> > -
> >  static int read_buffer_show(struct seq_file *s_file, void *data)
> >  {
> > -	char buf[MAX_LINE_LEN];
> >  	int i;
> > +	char *buf __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> 
> IMO, it's better to scratch the whole previous change.
> It makes little sense to allocate a large buffer here just for the
> temporary formatting.
> 
> Moreover, *_show() functions there take seq_file pointer, and you can
> just use seq_printf() directly.  The sysfs/kobject show callback is
> with the a fixed PAGE_SIZE buffer, hence sysfs_emit() is useful, but
> in this case, it's a completely different story.

Agree, dropped now!

-- 
~Vinod

Re: [PATCH] soundwire: sysfs_emit() expects a buffer of size PAGE_SIZE

Posted by Sergio Pérez 2 months, 1 week ago

All right, then, this will be handled with a different approach.

Thanks,
Sergio

El jue, 24 jul 2025 a la(s) 3:52 a.m., Vinod Koul (vkoul@kernel.org) escribió:
>
> On 24-07-25, 09:19, Takashi Iwai wrote:
> > On Thu, 24 Jul 2025 01:09:25 +0200,
> > Sergio Perez Gonzalez wrote:
> > >
> > > In read_buffer_show(), allocate sufficient memory to pass on to
> > > sysfs_emit(), which expects a buffer of size PAGE_SIZE.
> > >
> > > Link: https://scan7.scan.coverity.com/#/project-view/53936/11354?selectedIssue=1648019
> > > Fixes: 35323d8ab811 ("soundwire: replace scnprintf() with sysfs_emit() for sysfs consistency")
> > > Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com>
> > > ---
> > >  drivers/soundwire/debugfs.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> > > index 825f7abbad32..08fb1e29026e 100644
> > > --- a/drivers/soundwire/debugfs.c
> > > +++ b/drivers/soundwire/debugfs.c
> > > @@ -306,12 +306,12 @@ static int cmd_go(void *data, u64 value)
> > >  DEFINE_DEBUGFS_ATTRIBUTE(cmd_go_fops, NULL,
> > >                      cmd_go, "%llu\n");
> > >
> > > -#define MAX_LINE_LEN 128
> > > -
> > >  static int read_buffer_show(struct seq_file *s_file, void *data)
> > >  {
> > > -   char buf[MAX_LINE_LEN];
> > >     int i;
> > > +   char *buf __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > > +   if (!buf)
> > > +           return -ENOMEM;
> >
> > IMO, it's better to scratch the whole previous change.
> > It makes little sense to allocate a large buffer here just for the
> > temporary formatting.
> >
> > Moreover, *_show() functions there take seq_file pointer, and you can
> > just use seq_printf() directly.  The sysfs/kobject show callback is
> > with the a fixed PAGE_SIZE buffer, hence sysfs_emit() is useful, but
> > in this case, it's a completely different story.
>
> Agree, dropped now!
>
> --
> ~Vinod