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: <5fadd85e-f2d7-878c-b709-3523e89dd93a@csgroup.eu>
Date: Thu, 28 Sep 2023 16:31:30 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: "j.granados@...sung.com" <j.granados@...sung.com>, Luis Chamberlain
	<mcgrof@...nel.org>, "willy@...radead.org" <willy@...radead.org>,
	"josh@...htriplett.org" <josh@...htriplett.org>, Kees Cook
	<keescook@...omium.org>, Phillip Potter <phil@...lpotter.co.uk>, Clemens
 Ladisch <clemens@...isch.de>, Arnd Bergmann <arnd@...db.de>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, Juergen Gross <jgross@...e.com>,
	Stefano Stabellini <sstabellini@...nel.org>, Oleksandr Tyshchenko
	<oleksandr_tyshchenko@...m.com>, Jiri Slaby <jirislaby@...nel.org>, "James
 E.J. Bottomley" <jejb@...ux.ibm.com>, "Martin K. Petersen"
	<martin.petersen@...cle.com>, Doug Gilbert <dgilbert@...erlog.com>, Sudip
 Mukherjee <sudipm.mukherjee@...il.com>, Jason Gunthorpe <jgg@...pe.ca>, Leon
 Romanovsky <leon@...nel.org>, Corey Minyard <minyard@....org>, Theodore Ts'o
	<tytso@....edu>, "Jason A. Donenfeld" <Jason@...c4.com>, David Ahern
	<dsahern@...nel.org>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Robin Holt <robinmholt@...il.com>, Steve Wahl
	<steve.wahl@....com>, Russ Weight <russell.h.weight@...el.com>, "Rafael J.
 Wysocki" <rafael@...nel.org>, Song Liu <song@...nel.org>, "K. Y. Srinivasan"
	<kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu
	<wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, Jani Nikula
	<jani.nikula@...ux.intel.com>, Joonas Lahtinen
	<joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>,
	Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>, David Airlie
	<airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>
CC: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-raid@...r.kernel.org" <linux-raid@...r.kernel.org>,
	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
	"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
	"openipmi-developer@...ts.sourceforge.net"
	<openipmi-developer@...ts.sourceforge.net>, "linuxppc-dev@...ts.ozlabs.org"
	<linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH 00/15] sysctl: Remove sentinel elements from drivers



Le 28/09/2023 à 15:21, Joel Granados via B4 Relay a écrit :
> From: Joel Granados <j.granados@...sung.com>

Automatic test fails on powerpc, see 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20230928-jag-sysctl_remove_empty_elem_drivers-v1-15-e59120fca9f9@samsung.com/

Kernel attempted to read user page (1a111316) - exploit attempt? (uid: 0)
BUG: Unable to handle kernel data access on read at 0x1a111316
Faulting instruction address: 0xc0545338
Oops: Kernel access of bad area, sig: 11 [#1]
BE PAGE_SIZE=4K PowerPC 44x Platform
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 6.5.0-rc6-gdef13277bacb #1
Hardware name: amcc,bamboo 440GR Rev. B 0x422218d3 PowerPC 44x Platform
NIP:  c0545338 LR: c0548468 CTR: ffffffff
REGS: c084fae0 TRAP: 0300   Not tainted  (6.5.0-rc6-gdef13277bacb)
MSR:  00021000 <CE,ME>  CR: 84004288  XER: 00000000
DEAR: 1a111316 ESR: 00000000
GPR00: c0548468 c084fbd0 c0888000 c084fc99 00000000 c084fc7c 1a110316 
000affff
GPR08: ffffffff c084fd18 1a111316 04ffffff 22000282 00000000 c00027c0 
00000000
GPR16: 00000000 00000000 c0040000 c003d544 00000001 c003eb2c 096023d4 
00000000
GPR24: c0636502 c0636502 c084fc74 c0588510 c084fc68 c084fc7c c084fc99 
00000002
NIP [c0545338] string+0x78/0x148
LR [c0548468] vsnprintf+0x3d8/0x824
Call Trace:
[c084fbd0] [c084fc7c] 0xc084fc7c (unreliable)
[c084fbe0] [c0548468] vsnprintf+0x3d8/0x824
[c084fc30] [c0072dec] vprintk_store+0x17c/0x4c8
[c084fcc0] [c007322c] vprintk_emit+0xf4/0x2a0
[c084fd00] [c0073d04] _printk+0x60/0x88
[c084fd40] [c01ab63c] sysctl_err+0x78/0xa4
[c084fd80] [c01ab404] __register_sysctl_table+0x6a0/0x6c4
[c084fde0] [c06a585c] __register_sysctl_init+0x30/0x78
[c084fe00] [c06a8cc8] tty_init+0x44/0x168
[c084fe30] [c00023c4] do_one_initcall+0x64/0x2a0
[c084fea0] [c068f060] kernel_init_freeable+0x184/0x230
[c084fee0] [c00027e4] kernel_init+0x24/0x124
[c084ff00] [c000f1fc] ret_from_kernel_user_thread+0x14/0x1c
--- interrupt: 0 at 0x0
NIP:  00000000 LR: 00000000 CTR: 00000000
REGS: c084ff10 TRAP: 0000   Not tainted  (6.5.0-rc6-gdef13277bacb)
MSR:  00000000 <>  CR: 00000000  XER: 00000000

GPR00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
GPR08: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
GPR24: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
NIP [00000000] 0x0
LR [00000000] 0x0
--- interrupt: 0
Code: 91610008 90e1000c 4bffd0b5 80010014 38210010 7c0803a6 4e800020 
409d0008 99230000 38630001 38840001 4240ffd0 <7d2a20ae> 7f851840 
5528063e 2c080000
---[ end trace 0000000000000000 ]---

note: swapper[1] exited with irqs disabled
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b


> 
> What?
> These commits remove the sentinel element (last empty element) from the
> sysctl arrays of all the files under the "drivers/" directory that use a
> sysctl array for registration. The merging of the preparation patches
> (in https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
> to mainline allows us to just remove sentinel elements without changing
> behavior (more info here [1]).
> 
> These commits are part of a bigger set (here
> https://github.com/Joelgranados/linux/tree/tag/sysctl_remove_empty_elem_V4)
> that remove the ctl_table sentinel. Make the review process easier by
> chunking the commits into manageable pieces. Each chunk can be reviewed
> separately without noise from parallel sets.
> 
> Now that the architecture chunk has been mostly reviewed [6], we send
> the "drivers/" directory. Once this one is done, it will be follwed by
> "fs/*", "kernel/*", "net/*" and miscellaneous. The final set will remove
> the unneeded check for ->procname == NULL.
> 
> Why?
> By removing the sysctl sentinel elements we avoid kernel bloat as
> ctl_table arrays get moved out of kernel/sysctl.c into their own
> respective subsystems. This move was started long ago to avoid merge
> conflicts; the sentinel removal bit came after Mathew Wilcox suggested
> it to avoid bloating the kernel by one element as arrays moved out. This
> patchset will reduce the overall build time size of the kernel and run
> time memory bloat by about ~64 bytes per declared ctl_table array. I
> have consolidated some links that shed light on the history of this
> effort [2].
> 
> Testing:
> * Ran sysctl selftests (./tools/testing/selftests/sysctl/sysctl.sh)
> * Ran this through 0-day with no errors or warnings
> 
> Size saving after removing all sentinels:
>    These are the bytes that we save after removing all the sentinels
>    (this plus all the other chunks). I included them to get an idea of
>    how much memory we are talking about.
>      * bloat-o-meter:
>          - The "yesall" configuration results save 9158 bytes
>            https://lore.kernel.org/all/20230621091000.424843-1-j.granados@samsung.com/
>          - The "tiny" config + CONFIG_SYSCTL save 1215 bytes
>            https://lore.kernel.org/all/20230809105006.1198165-1-j.granados@samsung.com/
>      * memory usage:
>          In memory savings are measured to be 7296 bytes. (here is how to
>          measure [3])
> 
> Size saving after this patchset:
>      * bloat-o-meter
>          - The "yesall" config saves 2432 bytes [4]
>          - The "tiny" config saves 64 bytes [5]
>      * memory usage:
>          In this case there were no bytes saved because I do not have any
>          of the drivers in the patch. To measure it comment the printk in
>          `new_dir` and uncomment the if conditional in `new_links` [3].
> 
> Comments/feedback greatly appreciated
> 
> Best
> Joel
> 
> [1]
> We are able to remove a sentinel table without behavioral change by
> introducing a table_size argument in the same place where procname is
> checked for NULL. The idea is for it to keep stopping when it hits
> ->procname == NULL, while the sentinel is still present. And when the
> sentinel is removed, it will stop on the table_size. You can go to
> (https://lore.kernel.org/all/20230809105006.1198165-1-j.granados@samsung.com/)
> for more information.
> 
> [2]
> Links Related to the ctl_table sentinel removal:
> * Good summary from Luis sent with the "pull request" for the
>    preparation patches.
>    https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/
> * Another very good summary from Luis.
>    https://lore.kernel.org/all/ZMFizKFkVxUFtSqa@bombadil.infradead.org/
> * This is a patch set that replaces register_sysctl_table with register_sysctl
>    https://lore.kernel.org/all/20230302204612.782387-1-mcgrof@kernel.org/
> * Patch set to deprecate register_sysctl_paths()
>    https://lore.kernel.org/all/20230302202826.776286-1-mcgrof@kernel.org/
> * Here there is an explicit expectation for the removal of the sentinel element.
>    https://lore.kernel.org/all/20230321130908.6972-1-frank.li@vivo.com
> * The "ARRAY_SIZE" approach was mentioned (proposed?) in this thread
>    https://lore.kernel.org/all/20220220060626.15885-1-tangmeng@uniontech.com
> 
> [3]
> To measure the in memory savings apply this on top of this patchset.
> 
> "
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index c88854df0b62..e0073a627bac 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -976,6 +976,8 @@ static struct ctl_dir *new_dir(struct ctl_table_set *set,
>          table[0].procname = new_name;
>          table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO;
>          init_header(&new->header, set->dir.header.root, set, node, table, 1);
> +       // Counts additional sentinel used for each new dir.
> +       printk("%ld sysctl saved mem kzalloc \n", sizeof(struct ctl_table));
> 
>          return new;
>   }
> @@ -1199,6 +1201,9 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table_
>                  link_name += len;
>                  link++;
>          }
> +       // Counts additional sentinel used for each new registration
> +       //if ((head->ctl_table + head->ctl_table_size)->procname)
> +               printk("%ld sysctl saved mem kzalloc \n", sizeof(struct ctl_table));
>          init_header(links, dir->header.root, dir->header.set, node, link_table,
>                      head->ctl_table_size);
>          links->nreg = nr_entries;
> "
> and then run the following bash script in the kernel:
> 
> accum=0
> for n in $(dmesg | grep kzalloc | awk '{print $3}') ; do
>      echo $n
>      accum=$(calc "$accum + $n")
> done
> echo $accum
> 
> [4]
> add/remove: 0/0 grow/shrink: 0/21 up/down: 0/-2432 (-2432)
> Function                                     old     new   delta
> xpc_sys_xpc_hb                               192     128     -64
> xpc_sys_xpc                                  128      64     -64
> vrf_table                                    128      64     -64
> ucma_ctl_table                               128      64     -64
> tty_table                                    192     128     -64
> sg_sysctls                                   128      64     -64
> scsi_table                                   128      64     -64
> random_table                                 448     384     -64
> raid_table                                   192     128     -64
> oa_table                                     192     128     -64
> mac_hid_files                                256     192     -64
> iwcm_ctl_table                               128      64     -64
> ipmi_table                                   128      64     -64
> hv_ctl_table                                 128      64     -64
> hpet_table                                   128      64     -64
> firmware_config_table                        192     128     -64
> cdrom_table                                  448     384     -64
> balloon_table                                128      64     -64
> parport_sysctl_template                      912     720    -192
> parport_default_sysctl_table                 584     136    -448
> parport_device_sysctl_template               776     136    -640
> Total: Before=429940038, After=429937606, chg -0.00%
> 
> [5]
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-64 (-64)
> Function                                     old     new   delta
> random_table                                 448     384     -64
> Total: Before=1885527, After=1885463, chg -0.00%
> 
> [6] https://lore.kernel.org/all/20230913-jag-sysctl_remove_empty_elem_arch-v2-0-d1bd13a29bae@samsung.com/
> 
> Signed-off-by: Joel Granados <j.granados@...sung.com>
> 
> ---
> 
> ---
> Joel Granados (15):
>        cdrom: Remove now superfluous sentinel element from ctl_table array
>        hpet: Remove now superfluous sentinel element from ctl_table array
>        xen: Remove now superfluous sentinel element from ctl_table array
>        tty: Remove now superfluous sentinel element from ctl_table array
>        scsi: Remove now superfluous sentinel element from ctl_table array
>        parport: Remove the now superfluous sentinel element from ctl_table array
>        macintosh: Remove the now superfluous sentinel element from ctl_table array
>        infiniband: Remove the now superfluous sentinel element from ctl_table array
>        char-misc: Remove the now superfluous sentinel element from ctl_table array
>        vrf: Remove the now superfluous sentinel element from ctl_table array
>        sgi-xp: Remove the now superfluous sentinel element from ctl_table array
>        fw loader: Remove the now superfluous sentinel element from ctl_table array
>        raid: Remove now superfluous sentinel element from ctl_table array
>        hyper-v/azure: Remove now superfluous sentinel element from ctl_table array
>        intel drm: Remove now superfluous sentinel element from ctl_table array
> 
>   drivers/base/firmware_loader/fallback_table.c |  3 +-
>   drivers/cdrom/cdrom.c                         |  3 +-
>   drivers/char/hpet.c                           |  3 +-
>   drivers/char/ipmi/ipmi_poweroff.c             |  3 +-
>   drivers/char/random.c                         |  3 +-
>   drivers/gpu/drm/i915/i915_perf.c              |  3 +-
>   drivers/hv/hv_common.c                        |  3 +-
>   drivers/infiniband/core/iwcm.c                |  3 +-
>   drivers/infiniband/core/ucma.c                |  3 +-
>   drivers/macintosh/mac_hid.c                   |  3 +-
>   drivers/md/md.c                               |  3 +-
>   drivers/misc/sgi-xp/xpc_main.c                |  6 ++--
>   drivers/net/vrf.c                             |  3 +-
>   drivers/parport/procfs.c                      | 42 ++++++++++++---------------
>   drivers/scsi/scsi_sysctl.c                    |  3 +-
>   drivers/scsi/sg.c                             |  3 +-
>   drivers/tty/tty_io.c                          |  3 +-
>   drivers/xen/balloon.c                         |  3 +-
>   18 files changed, 36 insertions(+), 60 deletions(-)
> ---
> base-commit: 0e945134b680040b8613e962f586d91b6d40292d
> change-id: 20230927-jag-sysctl_remove_empty_elem_drivers-f034962a0d8c
> 
> Best regards,

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ