linux-kernel - Re: [PATCH 2/6] parport: Remove register_sysctl_table from parport_proc

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]

Message-ID: <20230516143116.hz6rr6kzsviobx35@localhost>
Date:   Tue, 16 May 2023 16:31:16 +0200
From:   Joel Granados <j.granados@...sung.com>
To:     Luis Chamberlain <mcgrof@...nel.org>
CC:     Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Iurii Zaikin <yzaikin@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        <linux-kernel@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH 2/6] parport: Remove register_sysctl_table from
 parport_proc_register

On Mon, May 15, 2023 at 09:17:39PM -0700, Luis Chamberlain wrote:
> Awesome!
> 
> On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> > +
> > +	port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> > +	/*
> > +	 * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> > +	 * We calculate for the second as that will give us enough for the first.
> > +	 */
> > +	tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> > +	tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
> 
> Any reason why not kzalloc()?
nope. Will zero it out.

> 
> > +	if (!tmp_dir_path) {
> > +		err = -ENOMEM;
> > +		goto exit_free_t;
> > +	}
> >  
> > -	t->port_dir[0].procname = port->name;
> > +	if (tmp_path_len
> > +	    <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
> 
> Since we are clearing up obfuscation code, it would be nicer to
> make this easier to read and split the snprintf() into one line, capture
> the error there. And then in a new line do the check. Even if we have to
> add a new int value here.
np. Will do something like this:

num_chars_sprinted = snprintf(....
if(tmp_path_len <= num_chars_sprinted) {
  err = -ENOENT;
  ...
}

> 
> Other than this I'd just ask to extend the commit log to use
> the before and after of vmlinux (when this module is compiled in with all
> the bells and whistles) with ./scripts/bloat-o-meter.
> 
> Ie build before the patch and cp vmlinux to vmlinux.old and then compare
> with:
> 
> ./scripts/bloat-o-meter vmlinux.old vmlinux
> 
> Can you also describe any testing if any.
Sure thing. Will add the bloat-o-meter output on the last patch so as to
gather the results for all the patches.

I'll write some testing info on the patches.


> 
> With the above changes, feel free to add to all these patches:
> 
> Reviewed-by: Luis Chamberlain
Ack

> 
> > +	if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
> 
> BTW, we should be able to remove now replace register_sysctl_base() with a simple
> register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
> on kernel/sysctl.c and just remove:
> 
>   * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
>   * and then after all this register_sysctl_table() completely
> 
> Let me know if you'd like a stab at it, or if you prefer me to do that.
I think I can give it a go. Should I just add that to these set of
patches? or should we create a new patch set?

> 
>   Luis
> 
> 

best
-- 

Joel Granados

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)