On Wed, 12 Oct 2022 07:58:20 +0900 (JST)
YASUOKA Masahiko <yasu...@openbsd.org> wrote:
> On Wed, 05 Oct 2022 13:37:35 +0900 (JST)
> Masato Asou <a...@soum.co.jp> wrote:
>> From: "Theo de Raadt" <dera...@openbsd.org>
>> Date: Tue, 04 Oct 2022 21:58:13 -0600
>>> Userland may not ask the kernel to allocate such a huge object. The
>>> kernel address space is quite small. First off, huge allocations will
>>> fail.
>>>
>>> But it is worse -- the small kernel address space is shared for many
>>> purposes, so large allocations will harm the other subsystems.
>>>
>>> As written, this diff is too dangerous. Arbitrary allocation inside
>>> the kernel is not reasonable. object sizes requested by userland must
>>> be small, or the operations must be cut up, which does create impact
>>> on atomicity or other things.
>>
>> As you pointed out, it is not a good idea to allocate large spaces
>> in kernel.
>>
>> Would it be better to keep the current fixed length?
>
> Currently the value on VMware may be truncated silently. It's simply
> broken. I think we should fix it by having a way to know if the value
> is reached the limit.
>
> Also I think we should be able to pass larger size of data. Since at
> least on VMware, people is useing for parameters when deployment
> through OVF tamplate. Sometimes the parameter includes large data
> like X.509 certificate.
>
> https://docs.vmware.com/en/VMware-vSphere/7.0/com.vmware.vsphere.vm_admin.doc/GUID-D0F9E3B9-B77B-4DEF-A982-49B9F6358FF3.html
>
> What do you think?
>
>> Prepare a variable like kern.maxpvbus and default it to
>> 4096. Futhermore, how about free() after copyout() to user space?
>
> I suppose we can use the space prepared by the userland directly.
I admit there is no way to use the user space directly in case of the
vmware.
But current vmt(4) uses the buffer in vmt_softc for all RPC commands.
The buffer seems to have beeen created for RPC done by the tclo
process. The tclo process executes RPC periodically, so having a long
term buffer does make sense.
On the otherhand, pvbus(4) prepares a buffer for
PVBUSIOC_KV{READ|WRITE} and pass it to the driver handler. So vmt(4)
can use the buffer.
The diff is to change vmt(4) to use the buffer given by pvbuf(4) for
the rpc output directly. Also make it return EMSGSIZE when the buffer
is not enough instead of truncating silently.
The diff is first step. We need to more hack for pvbus(4) and
vmt(4). For example, the buffer size pvbuf(4) allocates is not enough
to store the size user requested, since vmt(4) neeeds extra 2 bytes
for the RPC output.
+ bcopy(value + 2, value, valuelen - 2);
But I'd like to commit this first.
ok?
Index: sys/dev/pv/vmt.c
===================================================================
RCS file: /var/cvs/openbsd/src/sys/dev/pv/vmt.c,v
retrieving revision 1.26
diff -u -p -r1.26 vmt.c
--- sys/dev/pv/vmt.c 8 Sep 2022 10:22:06 -0000 1.26
+++ sys/dev/pv/vmt.c 19 Nov 2022 04:13:47 -0000
@@ -277,7 +277,8 @@ int vm_rpc_send(const struct vm_rpc *,
int vm_rpc_send_str(const struct vm_rpc *, const uint8_t *);
int vm_rpc_get_length(const struct vm_rpc *, uint32_t *, uint16_t *);
int vm_rpc_get_data(const struct vm_rpc *, char *, uint32_t, uint16_t);
-int vm_rpc_send_rpci_tx_buf(struct vmt_softc *, const uint8_t *, uint32_t);
+int vm_rpc_send_rpci_tx_buf(struct vmt_softc *, const uint8_t *, uint32_t,
+ uint8_t *, uint32_t);
int vm_rpc_send_rpci_tx(struct vmt_softc *, const char *, ...)
__attribute__((__format__(__kprintf__,2,3)));
int vm_rpci_response_successful(struct vmt_softc *);
@@ -491,7 +492,7 @@ int
vmt_kvop(void *arg, int op, char *key, char *value, size_t valuelen)
{
struct vmt_softc *sc = arg;
- char *buf = NULL, *ptr;
+ char *buf = NULL;
size_t bufsz;
int error = 0;
@@ -520,10 +521,9 @@ vmt_kvop(void *arg, int op, char *key, c
goto done;
}
- if (vm_rpc_send_rpci_tx(sc, "%s", buf) != 0) {
- DPRINTF("%s: error sending command: %s\n", DEVNAME(sc), buf);
+ if ((error = vm_rpc_send_rpci_tx_buf(sc, buf, strlen(buf), value,
+ valuelen)) != 0) {
sc->sc_rpc_error = 1;
- error = EIO;
goto done;
}
@@ -534,11 +534,7 @@ vmt_kvop(void *arg, int op, char *key, c
}
/* skip response that was tested in vm_rpci_response_successful() */
- ptr = sc->sc_rpc_buf + 2;
-
- /* might truncate, copy anyway but return error */
- if (strlcpy(value, ptr, valuelen) >= valuelen)
- error = ENOMEM;
+ bcopy(value + 2, value, valuelen - 2);
done:
free(buf, M_TEMP, bufsz);
@@ -1348,8 +1344,8 @@ vmt_nicinfo_task(void *data)
if (nic_info_size != sc->sc_nic_info_size ||
(memcmp(nic_info, sc->sc_nic_info, nic_info_size) != 0)) {
- if (vm_rpc_send_rpci_tx_buf(sc, nic_info,
- nic_info_size) != 0) {
+ if (vm_rpc_send_rpci_tx_buf(sc, nic_info, nic_info_size,
+ sc->sc_rpc_buf, VMT_RPC_BUFLEN) != 0) {
DPRINTF("%s: unable to send nic info",
DEVNAME(sc));
sc->sc_rpc_error = 1;
@@ -1651,7 +1647,7 @@ vm_rpci_response_successful(struct vmt_s
int
vm_rpc_send_rpci_tx_buf(struct vmt_softc *sc, const uint8_t *buf,
- uint32_t length)
+ uint32_t length, uint8_t *out, uint32_t outsz)
{
struct vm_rpc rpci;
u_int32_t rlen;
@@ -1677,16 +1673,18 @@ vm_rpc_send_rpci_tx_buf(struct vmt_softc
}
if (rlen > 0) {
- if (rlen >= VMT_RPC_BUFLEN) {
- rlen = VMT_RPC_BUFLEN - 1;
+ if (rlen + 1 > outsz) {
+ result = EMSGSIZE;
+ goto out;
}
- if (vm_rpc_get_data(&rpci, sc->sc_rpc_buf, rlen, ack) != 0) {
+ if (vm_rpc_get_data(&rpci, out, rlen, ack) != 0) {
DPRINTF("%s: failed to get rpci response data\n",
DEVNAME(sc));
result = EIO;
goto out;
}
+ out[rlen] = '\0';
}
out:
@@ -1712,7 +1710,8 @@ vm_rpc_send_rpci_tx(struct vmt_softc *sc
return EIO;
}
- return vm_rpc_send_rpci_tx_buf(sc, sc->sc_rpc_buf, len);
+ return vm_rpc_send_rpci_tx_buf(sc, sc->sc_rpc_buf, len, sc->sc_rpc_buf,
+ VMT_RPC_BUFLEN);
}
#if 0
