Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7498668e-0e91-be2b-daf2-a45bf0cf40c9@gmail.com>
Date:   Fri, 5 Aug 2022 10:33:02 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] lib/cmdline: avoid page fault in next_arg
Content-Language: en-US
To:     Neel Natu <neelnatu@google.com>, linux-kernel@vger.kernel.org
References: <20220728232434.1666488-1-neelnatu@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>
From:   Eric Dumazet <eric.dumazet@gmail.com>
In-Reply-To: <20220728232434.1666488-1-neelnatu@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/28/22 16:24, Neel Natu wrote:
> An argument list like "arg=val arg2 \"" can trigger a page fault
> if the page pointed by 'args[0xffffffff]' is not mapped and potential
> memory corruption otherwise (unlikely but possible if the bogus
> address is mapped and contents happen to match the ascii value
> of the quote character).
>
> The fix is to ensure that we load 'args[i-1]' only when (i > 0).
>
> Prior to this commit the following command would trigger an
> unhandled page fault in the kernel:
>
> root@(none):/linus/fs/fat# insmod ./fat.ko  "foo=bar \""
> [   33.870507] BUG: unable to handle page fault for address: ffff888204252608
> [   33.872180] #PF: supervisor read access in kernel mode
> [   33.873414] #PF: error_code(0x0000) - not-present page
> [   33.874650] PGD 4401067 P4D 4401067 PUD 0
> [   33.875321] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC PTI
> [   33.876113] CPU: 16 PID: 399 Comm: insmod Not tainted 5.19.0-dbg-DEV #4
> [   33.877193] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
> [   33.878739] RIP: 0010:next_arg+0xd1/0x110
> [   33.879399] Code: 22 75 1d 41 c6 04 01 00 41 80 f8 22 74 18 eb 35 4c 89 0e 45 31 d2 4c 89 cf 48 c7 02 00 00 00 00 41 80 f8 22 75 1f 41 8d 42 ff <41> 80 3c 01 22 75 14 41 c6 04 01 00 eb 0d 48 c7 02 00 00 00 00 41
> [   33.882338] RSP: 0018:ffffc90001253d08 EFLAGS: 00010246
> [   33.883174] RAX: 00000000ffffffff RBX: ffff888104252608 RCX: 0fc317bba1c1dd00
> [   33.884311] RDX: ffffc90001253d40 RSI: ffffc90001253d48 RDI: ffff888104252609
> [   33.885450] RBP: ffffc90001253d10 R08: 0000000000000022 R09: ffff888104252609
> [   33.886595] R10: 0000000000000000 R11: ffffffff82c7ff20 R12: 0000000000000282
> [   33.887748] R13: 00000000ffff8000 R14: 0000000000000000 R15: 0000000000007fff
> [   33.888887] FS:  00007f04ec7432c0(0000) GS:ffff88813d300000(0000) knlGS:0000000000000000
> [   33.890183] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   33.891111] CR2: ffff888204252608 CR3: 0000000100f36005 CR4: 0000000000170ee0
> [   33.892241] Call Trace:
> [   33.892641]  <TASK>
> [   33.892989]  parse_args+0x8f/0x220
> [   33.893538]  load_module+0x138b/0x15a0
> [   33.894149]  ? prepare_coming_module+0x50/0x50
> [   33.894879]  ? kernel_read_file_from_fd+0x5f/0x90
> [   33.895639]  __se_sys_finit_module+0xce/0x130
> [   33.896342]  __x64_sys_finit_module+0x1d/0x20
> [   33.897042]  do_syscall_64+0x44/0xa0
> [   33.897622]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [   33.898434] RIP: 0033:0x7f04ec85ef79
> [   33.899009] Code: 48 8d 3d da db 0d 00 0f 05 eb a5 66 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c7 9e 0d 00 f7 d8 64 89 01 48
> [   33.901912] RSP: 002b:00007fffae81bfe8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [   33.903081] RAX: ffffffffffffffda RBX: 0000559c5f1d2640 RCX: 00007f04ec85ef79
> [   33.904191] RDX: 0000000000000000 RSI: 0000559c5f1d12a0 RDI: 0000000000000003
> [   33.905304] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> [   33.906421] R10: 0000000000000003 R11: 0000000000000246 R12: 0000559c5f1d12a0
> [   33.907526] R13: 0000000000000000 R14: 0000559c5f1d25f0 R15: 0000559c5f1d12a0
> [   33.908631]  </TASK>
> [   33.908986] Modules linked in: fat(+) [last unloaded: fat]
> [   33.909843] CR2: ffff888204252608
> [   33.910375] ---[ end trace 0000000000000000 ]---
> [   33.911172] RIP: 0010:next_arg+0xd1/0x110
> [   33.911796] Code: 22 75 1d 41 c6 04 01 00 41 80 f8 22 74 18 eb 35 4c 89 0e 45 31 d2 4c 89 cf 48 c7 02 00 00 00 00 41 80 f8 22 75 1f 41 8d 42 ff <41> 80 3c 01 22 75 14 41 c6 04 01 00 eb 0d 48 c7 02 00 00 00 00 41
> [   33.914643] RSP: 0018:ffffc90001253d08 EFLAGS: 00010246
> [   33.915446] RAX: 00000000ffffffff RBX: ffff888104252608 RCX: 0fc317bba1c1dd00
> [   33.916544] RDX: ffffc90001253d40 RSI: ffffc90001253d48 RDI: ffff888104252609
> [   33.917636] RBP: ffffc90001253d10 R08: 0000000000000022 R09: ffff888104252609
> [   33.918727] R10: 0000000000000000 R11: ffffffff82c7ff20 R12: 0000000000000282
> [   33.919821] R13: 00000000ffff8000 R14: 0000000000000000 R15: 0000000000007fff
> [   33.920908] FS:  00007f04ec7432c0(0000) GS:ffff88813d300000(0000) knlGS:0000000000000000
> [   33.922125] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   33.923017] CR2: ffff888204252608 CR3: 0000000100f36005 CR4: 0000000000170ee0
> [   33.924098] Kernel panic - not syncing: Fatal exception
> [   33.925776] Kernel Offset: disabled
> [   33.926347] Rebooting in 10 seconds..
>
> Signed-off-by: Neel Natu <neelnatu@google.com>


Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks.


> ---
>   lib/cmdline.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index 5546bf588780..90ed997d9570 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -260,7 +260,7 @@ char *next_arg(char *args, char **param, char **val)
>   				args[i-1] = '\0';
>   		}
>   	}
> -	if (quoted && args[i-1] == '"')
> +	if (quoted && i > 0 && args[i-1] == '"')
>   		args[i-1] = '\0';
>   
>   	if (args[i]) {