Changeset View
Standalone View
sbin/ggate/ggatec/ggatec.c
| Show First 20 Lines • Show All 138 Lines • ▼ Show 20 Lines | #endif | ||||
| switch (ggio.gctl_cmd) { | switch (ggio.gctl_cmd) { | ||||
| case BIO_READ: | case BIO_READ: | ||||
| hdr.gh_cmd = GGATE_CMD_READ; | hdr.gh_cmd = GGATE_CMD_READ; | ||||
| break; | break; | ||||
| case BIO_WRITE: | case BIO_WRITE: | ||||
| hdr.gh_cmd = GGATE_CMD_WRITE; | hdr.gh_cmd = GGATE_CMD_WRITE; | ||||
| break; | break; | ||||
| default: | |||||
jo_bruelltuete.com: This is the real fix: fail requests that are not read or write. ie fail delete or flush… | |||||
| g_gate_log(LOG_NOTICE, "Unknown gctl_cmd: %i", ggio.gctl_cmd); | |||||
| ggio.gctl_error = EOPNOTSUPP; | |||||
| g_gate_ioctl(G_GATE_CMD_DONE, &ggio); | |||||
| continue; | |||||
| } | } | ||||
| /* Don't send requests for more data than we can handle the response for! */ | |||||
| if (ggio.gctl_length > MAXPHYS) { | |||||
Done Inline ActionsThis is just hardening. jo_bruelltuete.com: This is just hardening. | |||||
Not Done Inline ActionsThe kernel can absolutely handle a read(2) or write(2) operation larger than MAXPHYS. It will just split it up before sending it to the disks. So unless you know of a particular failure that a long request can trigger, I would not preemptively add this check. asomers: The kernel can absolutely handle a read(2) or write(2) operation larger than MAXPHYS. It will… | |||||
Done Inline ActionsThe failure is in ggatec handling the over-sized response from ggated, see the other change below. If ggatec sends off a large request, ggated will dutifully respond with a large response but that response will not fit into the statically sized buffer of ggatec and overflow. By failing that request here (instead of crashing out later) we are dealing with it and letting the requesting process now that it failed (otherwise it'll just hang). jo_bruelltuete.com: The failure is in ggatec handling the over-sized response from ggated, see the other change… | |||||
Not Done Inline ActionsAs noted above, this should be "sizeof(buf)". peterj: As noted above, this should be "sizeof(buf)". | |||||
Done Inline ActionsNo, that's misleading: the size of this buf here (the send buf) is not what matters. This buf is a separate local variable, distinct from the buf in recv_thread. jo_bruelltuete.com: No, that's misleading: the size of this buf here (the send buf) is not what matters. This buf… | |||||
Not Done Inline ActionsIn that case, I think the existing comment should be augmented to make it clear that the test here is against the size of "buf" in recv_thread. peterj: In that case, I think the existing comment should be augmented to make it clear that the test… | |||||
| g_gate_log(LOG_ERR, "Request too big: %zd", ggio.gctl_length); | |||||
| ggio.gctl_error = EOPNOTSUPP; | |||||
Not Done Inline ActionsI'm not familiar with the kernel side of geom_gate but I would expect ENOMEM to be more appropriate here. peterj: I'm not familiar with the kernel side of geom_gate but I would expect ENOMEM to be more… | |||||
Done Inline ActionsFrom user-mode pov of ggatec "not supported" seems good: I do not support this large request. jo_bruelltuete.com: From user-mode pov of ggatec "not supported" seems good: I do not support this large request. | |||||
Not Done Inline ActionsMy point is that this error code is processed by the kernel code and needs to chosen with that in mind. I did some digging and the error code is passed into sys/geom/geom_io.c:g_io_deliver() and that code treats ENOMEM specially. The error code will then be forwarded to the bio originator. peterj: My point is that this error code is processed by the kernel code and needs to chosen with that… | |||||
| g_gate_ioctl(G_GATE_CMD_DONE, &ggio); | |||||
| continue; | |||||
| } | |||||
| hdr.gh_seq = ggio.gctl_seq; | hdr.gh_seq = ggio.gctl_seq; | ||||
| hdr.gh_offset = ggio.gctl_offset; | hdr.gh_offset = ggio.gctl_offset; | ||||
| hdr.gh_length = ggio.gctl_length; | hdr.gh_length = ggio.gctl_length; | ||||
| hdr.gh_error = 0; | hdr.gh_error = 0; | ||||
| g_gate_swap2n_hdr(&hdr); | g_gate_swap2n_hdr(&hdr); | ||||
| data = g_gate_send(sendfd, &hdr, sizeof(hdr), MSG_NOSIGNAL); | data = g_gate_send(sendfd, &hdr, sizeof(hdr), MSG_NOSIGNAL); | ||||
| g_gate_log(LOG_DEBUG, "Sent hdr packet."); | g_gate_log(LOG_DEBUG, "Sent hdr packet."); | ||||
| ▲ Show 20 Lines • Show All 55 Lines • ▼ Show 20 Lines | for (;;) { | ||||
| } | } | ||||
| g_gate_log(LOG_DEBUG, "Received hdr packet."); | g_gate_log(LOG_DEBUG, "Received hdr packet."); | ||||
| ggio.gctl_seq = hdr.gh_seq; | ggio.gctl_seq = hdr.gh_seq; | ||||
| ggio.gctl_cmd = hdr.gh_cmd; | ggio.gctl_cmd = hdr.gh_cmd; | ||||
| ggio.gctl_offset = hdr.gh_offset; | ggio.gctl_offset = hdr.gh_offset; | ||||
| ggio.gctl_length = hdr.gh_length; | ggio.gctl_length = hdr.gh_length; | ||||
| ggio.gctl_error = hdr.gh_error; | ggio.gctl_error = hdr.gh_error; | ||||
| /* Do not overflow our buffer if there is a bogus response. */ | |||||
| if (ggio.gctl_length > (off_t) sizeof(buf)) { | |||||
Done Inline ActionsThis has security implications. I've emailed secteam@ but got no response. jo_bruelltuete.com: This has security implications. I've emailed secteam@ but got no response. | |||||
Not Done Inline ActionsWhere does the buffer overflow happen exactly? MAXPHYS normally only matters in-kernel. It can also vary at runtime on FreeBSD 13, which makes this usage suspicious. asomers: Where does the buffer overflow happen exactly? MAXPHYS normally only matters in-kernel. It… | |||||
Done Inline ActionsSee local variable char buf[MAXPHYS] up top. It's statically sized. Any malformed (intentional or not) response from ggated (or someone in a priviledged network position) will overwrite stack. jo_bruelltuete.com: See local variable //char buf[MAXPHYS]// up top. It's statically sized. Any malformed… | |||||
Not Done Inline ActionsIn that case the condition should be if (ggio.gctl_length > sizeof(buf)). And wow, this is quite the classic buffer overflow. asomers: In that case the condition should be `if (ggio.gctl_length > sizeof(buf))`. And wow, this is… | |||||
Done Inline Actions
Got any pointers? Is it a boot-time-config? jo_bruelltuete.com: > MAXPHYS normally only matters in-kernel. It can also vary at runtime on FreeBSD 13
Got any… | |||||
| g_gate_log(LOG_ERR, "Received too big response: %zd", ggio.gctl_length); | |||||
| break; | |||||
| } | |||||
| if (ggio.gctl_error == 0 && ggio.gctl_cmd == GGATE_CMD_READ) { | if (ggio.gctl_error == 0 && ggio.gctl_cmd == GGATE_CMD_READ) { | ||||
| data = g_gate_recv(recvfd, ggio.gctl_data, | data = g_gate_recv(recvfd, ggio.gctl_data, | ||||
| ggio.gctl_length, MSG_WAITALL); | ggio.gctl_length, MSG_WAITALL); | ||||
| if (reconnect) | if (reconnect) | ||||
| break; | break; | ||||
| g_gate_log(LOG_DEBUG, "Received data packet."); | g_gate_log(LOG_DEBUG, "Received data packet."); | ||||
| if (data != ggio.gctl_length) { | if (data != ggio.gctl_length) { | ||||
| ▲ Show 20 Lines • Show All 419 Lines • Show Last 20 Lines | |||||
This is the real fix: fail requests that are not read or write. ie fail delete or flush requests.
Without this a full device delete (for example initiated by zpool create) would create a bogus read (or write or whatever) request with a huge size.