linux-kernel - Re: [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 4 Sep 2023 16:44:09 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Joel Granados <j.granados@...sung.com>
Cc:     Luis Chamberlain <mcgrof@...nel.org>, linux-kernel@...r.kernel.org,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>
Subject: Re: [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer
 formatting

On Mon, Sep 04, 2023 at 03:11:45PM +0200, Joel Granados wrote:
> On Fri, Sep 01, 2023 at 04:42:48PM +0300, Andy Shevchenko wrote:

Thank you for the review, my answers below.

...

> > @@ -431,8 +424,7 @@ int parport_proc_register(struct parport *port)
> >  {
> >  	struct parport_sysctl_table *t;
> >  	char *tmp_dir_path;
> > -	size_t tmp_path_len, port_name_len;
> > -	int bytes_written, i, err = 0;
> > +	int i, err = 0;
> >  
> >  	t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL);
> >  	if (t == NULL)
> > @@ -446,35 +438,23 @@ int parport_proc_register(struct parport *port)
> For this function I would even go a step further and start with the two
> kasprintf calls so we can then free them in the reverse order. And then
> leave the rest as it is.

I'm not sure I see the big picture here. Can you draft what you are proposing
(showing only the lines that are important)?

> I attached an untested diff that applies on
> top of your changes to show you what I mean.

Ah, I see now, it's below. And how is it better?
(LoCs statistics seems to be the same, so...)

What is a downside in my opinion with your code is this line

	return 0; --> goto blablabla.

this makes code less maintainable.

OTOH we may do what you want, but it will take a bit more LoCs and honestly
I don't see the benefit of doing that as in both cases the variable used is
temporary. What may be the good solution here is to split the repetitive
code excerpt to the parameterized helper function. But this will be another
patch which you can build on top of this series, right?

...

> > diff --git a/include/linux/parport.h b/include/linux/parport.h
> > index 999eddd619b7..fff39bc30629 100644
> > --- a/include/linux/parport.h
> > +++ b/include/linux/parport.h
> > @@ -180,8 +180,6 @@ struct ieee1284_info {
> >  	struct semaphore irq;
> >  };
> >  
> > -#define PARPORT_NAME_MAX_LEN 15
> This variable protected against port->name not ending in '\0'. Anyone
> worried that kasprintf could be unbounded?

I'm lost here. kasprintf() guarantees the NUL-termination. Any other concerns?

-- 
With Best Regards,
Andy Shevchenko