| Message ID | 20240319-u-boot-fix-p200-serial-v1-1-9a4e06815de0@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | board: amlogic: fix buffler overflow in serial & usid read | expand |
On Tue, Mar 19, 2024 at 03:53:24PM +0100, Neil Armstrong wrote: > While meson_sm_read_efuse() doesn't overflow, the string is not > zero terminated and env_set() will buffer overflow and add random > characters to environment. > In the Linux kernel we would give this a CVE because it's information disclosure bug... > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > board/amlogic/jethub-j80/jethub-j80.c | 6 ++++-- > board/amlogic/p200/p200.c | 3 ++- > board/amlogic/p201/p201.c | 3 ++- > board/amlogic/p212/p212.c | 3 ++- > board/amlogic/q200/q200.c | 3 ++- > 5 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/board/amlogic/jethub-j80/jethub-j80.c b/board/amlogic/jethub-j80/jethub-j80.c > index 185880de13..d10492cc46 100644 > --- a/board/amlogic/jethub-j80/jethub-j80.c > +++ b/board/amlogic/jethub-j80/jethub-j80.c > @@ -28,8 +28,8 @@ > int misc_init_r(void) > { > u8 mac_addr[EFUSE_MAC_SIZE]; ^^^^^^^^^^^^^^^^^^^^^^^^ This one is also a problem. You can't pass non-terminated strings to eth_env_set_enetaddr(). We call strlen() on it in either hsearch_r() or env_get_from_linear(). All the other functions had a mac_addr[] issue as well. Btw, this kind of bug is a good candidate for a static checker warning. I can create a Smatch check for this. It would probably be easier in Coccinelle even, but I'm the Smatch maintainer. regards, dan carpenter > - char serial[EFUSE_SN_SIZE]; > - char usid[EFUSE_USID_SIZE]; > + char serial[EFUSE_SN_SIZE + 1]; > + char usid[EFUSE_USID_SIZE + 1]; > ssize_t len; > unsigned int adcval; > int ret; > @@ -46,6 +46,7 @@ int misc_init_r(void) > if (!env_get("serial")) { > len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial, > EFUSE_SN_SIZE); > + serial[len] = '\0'; > if (len == EFUSE_SN_SIZE) > env_set("serial", serial);
On 20/03/2024 06:28, Dan Carpenter wrote: > On Tue, Mar 19, 2024 at 03:53:24PM +0100, Neil Armstrong wrote: >> While meson_sm_read_efuse() doesn't overflow, the string is not >> zero terminated and env_set() will buffer overflow and add random >> characters to environment. >> > > In the Linux kernel we would give this a CVE because it's information > disclosure bug... Yes probably > >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> board/amlogic/jethub-j80/jethub-j80.c | 6 ++++-- >> board/amlogic/p200/p200.c | 3 ++- >> board/amlogic/p201/p201.c | 3 ++- >> board/amlogic/p212/p212.c | 3 ++- >> board/amlogic/q200/q200.c | 3 ++- >> 5 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/board/amlogic/jethub-j80/jethub-j80.c b/board/amlogic/jethub-j80/jethub-j80.c >> index 185880de13..d10492cc46 100644 >> --- a/board/amlogic/jethub-j80/jethub-j80.c >> +++ b/board/amlogic/jethub-j80/jethub-j80.c >> @@ -28,8 +28,8 @@ >> int misc_init_r(void) >> { >> u8 mac_addr[EFUSE_MAC_SIZE]; > ^^^^^^^^^^^^^^^^^^^^^^^^ > > This one is also a problem. You can't pass non-terminated strings to > eth_env_set_enetaddr(). We call strlen() on it in either hsearch_r() or > env_get_from_linear(). > > All the other functions had a mac_addr[] issue as well. Ack, I'll also fix those, I should have checked before... > > Btw, this kind of bug is a good candidate for a static checker warning. > I can create a Smatch check for this. It would probably be easier in > Coccinelle even, but I'm the Smatch maintainer. Would be nice! > > regards, > dan carpenter > > >> - char serial[EFUSE_SN_SIZE]; >> - char usid[EFUSE_USID_SIZE]; >> + char serial[EFUSE_SN_SIZE + 1]; >> + char usid[EFUSE_USID_SIZE + 1]; >> ssize_t len; >> unsigned int adcval; >> int ret; >> @@ -46,6 +46,7 @@ int misc_init_r(void) >> if (!env_get("serial")) { >> len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial, >> EFUSE_SN_SIZE); >> + serial[len] = '\0'; >> if (len == EFUSE_SN_SIZE) >> env_set("serial", serial); > Thanks, Neil
On Wed, Mar 20, 2024 at 09:26:29AM +0100, Neil Armstrong wrote: > On 20/03/2024 06:28, Dan Carpenter wrote: > > On Tue, Mar 19, 2024 at 03:53:24PM +0100, Neil Armstrong wrote: > > > While meson_sm_read_efuse() doesn't overflow, the string is not > > > zero terminated and env_set() will buffer overflow and add random > > > characters to environment. > > > > > > > In the Linux kernel we would give this a CVE because it's information > > disclosure bug... > > Yes probably Yes, but this isn't the Linux kernel and we aren't a CNA. I don't object to someone getting a CVE if so inclined, but we don't have the resources to follow in the kernel's footsteps here either.
diff --git a/board/amlogic/jethub-j80/jethub-j80.c b/board/amlogic/jethub-j80/jethub-j80.c index 185880de13..d10492cc46 100644 --- a/board/amlogic/jethub-j80/jethub-j80.c +++ b/board/amlogic/jethub-j80/jethub-j80.c @@ -28,8 +28,8 @@ int misc_init_r(void) { u8 mac_addr[EFUSE_MAC_SIZE]; - char serial[EFUSE_SN_SIZE]; - char usid[EFUSE_USID_SIZE]; + char serial[EFUSE_SN_SIZE + 1]; + char usid[EFUSE_USID_SIZE + 1]; ssize_t len; unsigned int adcval; int ret; @@ -46,6 +46,7 @@ int misc_init_r(void) if (!env_get("serial")) { len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial, EFUSE_SN_SIZE); + serial[len] = '\0'; if (len == EFUSE_SN_SIZE) env_set("serial", serial); } @@ -53,6 +54,7 @@ int misc_init_r(void) if (!env_get("usid")) { len = meson_sm_read_efuse(EFUSE_USID_OFFSET, usid, EFUSE_USID_SIZE); + usid[len] = '\0'; if (len == EFUSE_USID_SIZE) env_set("usid", usid); } diff --git a/board/amlogic/p200/p200.c b/board/amlogic/p200/p200.c index 7c432f9d28..37a54e715c 100644 --- a/board/amlogic/p200/p200.c +++ b/board/amlogic/p200/p200.c @@ -22,7 +22,7 @@ int misc_init_r(void) { u8 mac_addr[EFUSE_MAC_SIZE]; - char serial[EFUSE_SN_SIZE]; + char serial[EFUSE_SN_SIZE + 1]; ssize_t len; if (!eth_env_get_enetaddr("ethaddr", mac_addr)) { @@ -35,6 +35,7 @@ int misc_init_r(void) if (!env_get("serial#")) { len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial, EFUSE_SN_SIZE); + serial[len] = '\0'; if (len == EFUSE_SN_SIZE) env_set("serial#", serial); } diff --git a/board/amlogic/p201/p201.c b/board/amlogic/p201/p201.c index 7c432f9d28..37a54e715c 100644 --- a/board/amlogic/p201/p201.c +++ b/board/amlogic/p201/p201.c @@ -22,7 +22,7 @@ int misc_init_r(void) { u8 mac_addr[EFUSE_MAC_SIZE]; - char serial[EFUSE_SN_SIZE]; + char serial[EFUSE_SN_SIZE + 1]; ssize_t len; if (!eth_env_get_enetaddr("ethaddr", mac_addr)) { @@ -35,6 +35,7 @@ int misc_init_r(void) if (!env_get("serial#")) { len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial, EFUSE_SN_SIZE); + serial[len] = '\0'; if (len == EFUSE_SN_SIZE) env_set("serial#", serial); } diff --git a/board/amlogic/p212/p212.c b/board/amlogic/p212/p212.c index fcef90bce5..90ac9f885d 100644 --- a/board/amlogic/p212/p212.c +++ b/board/amlogic/p212/p212.c @@ -23,7 +23,7 @@ int misc_init_r(void) { u8 mac_addr[EFUSE_MAC_SIZE]; - char serial[EFUSE_SN_SIZE]; + char serial[EFUSE_SN_SIZE + 1]; ssize_t len; if (!eth_env_get_enetaddr("ethaddr", mac_addr)) { @@ -38,6 +38,7 @@ int misc_init_r(void) if (!env_get("serial#")) { len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial, EFUSE_SN_SIZE); + serial[len] = '\0'; if (len == EFUSE_SN_SIZE) env_set("serial#", serial); } diff --git a/board/amlogic/q200/q200.c b/board/amlogic/q200/q200.c index 3aa6d8f200..1c47f4645f 100644 --- a/board/amlogic/q200/q200.c +++ b/board/amlogic/q200/q200.c @@ -23,7 +23,7 @@ int misc_init_r(void) { u8 mac_addr[EFUSE_MAC_SIZE]; - char serial[EFUSE_SN_SIZE]; + char serial[EFUSE_SN_SIZE + 1]; ssize_t len; if (!eth_env_get_enetaddr("ethaddr", mac_addr)) { @@ -38,6 +38,7 @@ int misc_init_r(void) if (!env_get("serial#")) { len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial, EFUSE_SN_SIZE); + serial[len] = '\0'; if (len == EFUSE_SN_SIZE) env_set("serial#", serial); }
While meson_sm_read_efuse() doesn't overflow, the string is not zero terminated and env_set() will buffer overflow and add random characters to environment. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- board/amlogic/jethub-j80/jethub-j80.c | 6 ++++-- board/amlogic/p200/p200.c | 3 ++- board/amlogic/p201/p201.c | 3 ++- board/amlogic/p212/p212.c | 3 ++- board/amlogic/q200/q200.c | 3 ++- 5 files changed, 12 insertions(+), 6 deletions(-) --- base-commit: b145877c22b391a4872c875145a8f86f6ffebaba change-id: 20240319-u-boot-fix-p200-serial-a017f57caf88 Best regards,