Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sending multiple Ctrl+C to UART ttylogin will trigger a system restart #11306

Open
angliu-muxi opened this issue Nov 21, 2022 · 4 comments · May be fixed by openwrt/procd#13
Open

Sending multiple Ctrl+C to UART ttylogin will trigger a system restart #11306

angliu-muxi opened this issue Nov 21, 2022 · 4 comments · May be fixed by openwrt/procd#13

Comments

@angliu-muxi
Copy link

first enable ttylogin

uci set system.@System[0].ttylogin='1'; uci commit; sync; reboot

second input root and send multiple Ctrl+C by UART, the system will reboot

OpenWrt login: ^C^CPlease press Enter to activate this console.
^CPlease press Enter to activate this console.
^CPlease press Enter to activate this console.
^CPlease press Enter to activate this console.
^C[  229.476790] reboot: Restarting system

It seems that during the time /usr/libexec/login.sh exited, procd received the Ctrl+C signal, causing procd to restart the system

ttyS0::askfirst:/usr/libexec/login.sh

@M95D
Copy link
Contributor

M95D commented Nov 28, 2022

Why is this a problem?
Even if your router asks for a password on the serial console, anyone with physical access can simply cut the power to reset the router.

@angliu-muxi
Copy link
Author

Why is this a problem? Even if your router asks for a password on the serial console, anyone with physical access can simply cut the power to reset the router.

The premise of system restart is that the system administrator knows what is being done, rather than the restart caused by unknown operations. According to the thinking of non-developers, who knows that Ctrl+C will cause system downtime. If the current system is performing core business, the consequences will be is catastrophic

@mcprat
Copy link
Contributor

mcprat commented Dec 8, 2022

non-developer would not be using UART

It would be nice if you or someone else could fix this, but I think we can all agree that it is not a critical problem

it seems like your description of the cause is pretty accurate, although fixing it might involve a kernel patch instead of changes to procd, since procd is not in control of what happens when it recieves SIGINT. alternatively, something about blocking or redirecting console to null might be possible, but IIRC, procd actually uses the console to interact with ubusd or something like that...

@nihilus
Copy link

nihilus commented Oct 7, 2024

non-developer would not be using UART

It would be nice if you or someone else could fix this, but I think we can all agree that it is not a critical problem

it seems like your description of the cause is pretty accurate, although fixing it might involve a kernel patch instead of changes to procd, since procd is not in control of what happens when it recieves SIGINT. alternatively, something about blocking or redirecting console to null might be possible, but IIRC, procd actually uses the console to interact with ubusd or something like that...

Well, the thing is that FAEs and support technicians might use console access.
When you have let's say 3 million deployed devices you don't want developers to do field maintenance...

The problem here is that procd gets a controlling TTY during a window period of ~0.5s, which is 100% unintended and a bug in procd.

I'll post a fix for the issue later this week after it has passed our internal code-review.

nihilus pushed a commit to nihilus/procd that referenced this issue Oct 8, 2024
Disable the use of implicit controlling
TTYs. They will be enabled on demand.

This fixes a bug where 2 or more
consecutive Ctrl-C at the login prompt
triggers a reboot of the device.

Closes: openwrt/openwrt#11306

Signed-off-by: Markus Gothe <[email protected]>
@nihilus nihilus linked a pull request Oct 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants