[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] slirp: Implement RFC2132 TFTP server name
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [PATCH v3] slirp: Implement RFC2132 TFTP server name |
Date: |
Mon, 27 Aug 2018 11:37:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 2018-08-27 10:36, Fam Zheng wrote:
> This new usernet option can be used to add data for option 66 (tftp
> server name) in the BOOTP reply, which is useful in PXE based automatic
> OS install such as OpenBSD.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>
> v3: Add doc to qemu-options.hx. [Thomas]
>
> v2: - Adjust parameter order and field placement to be closer to other
> tftp options. [Samuel]
> - Drop 'optional'. [Eric]
> ---
> net/slirp.c | 5 ++++-
> qapi/net.json | 5 ++++-
> qemu-options.hx | 7 ++++++-
> slirp/bootp.c | 8 ++++++++
> slirp/bootp.h | 1 +
> slirp/libslirp.h | 4 +++-
> slirp/slirp.c | 5 ++++-
> slirp/slirp.h | 1 +
> 8 files changed, 31 insertions(+), 5 deletions(-)
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 5515dfaba5..6ddabd9b81 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1885,7 +1885,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
> " [,ipv6[=on|off]][,ipv6-net=addr[/int]][,ipv6-host=addr]\n"
> " [,restrict=on|off][,hostname=host][,dhcpstart=addr]\n"
> "
> [,dns=addr][,ipv6-dns=addr][,dnssearch=domain][,domainname=domain]\n"
> - " [,tftp=dir][,bootfile=f][,hostfwd=rule][,guestfwd=rule]"
> + "
> [,tftp=dir][,tftp-server-name][,bootfile=f][,hostfwd=rule][,guestfwd=rule]"
Please use [,tftp-server-name=name] or [,tftp-server-name=n] or
something similar.
> #ifndef _WIN32
> "[,smb=dir[,smbserver=addr]]\n"
> #endif
> @@ -2122,6 +2122,11 @@ server. The files in @var{dir} will be exposed as the
> root of a TFTP server.
> The TFTP client on the guest must be configured in binary mode (use the
> command
> @code{bin} of the Unix TFTP client).
>
> address@hidden address@hidden
> +In BOOTP reply, broadcast @var{name} as the "TFTP server name" (RFC2132
> option
> +66). This can be used to advise the guest to load boot files or
> configurations
> +from a different server than the host address.
> +
> @item address@hidden
> When using the user mode network stack, broadcast @var{file} as the BOOTP
> filename. In conjunction with @option{tftp}, this can be used to network boot
> diff --git a/slirp/bootp.c b/slirp/bootp.c
> index 9e7b53ba94..ed2cf3b96e 100644
> --- a/slirp/bootp.c
> +++ b/slirp/bootp.c
> @@ -306,6 +306,14 @@ static void bootp_reply(Slirp *slirp, const struct
> bootp_t *bp)
> q += val;
> }
>
> + if (slirp->tftp_server_name) {
> + val = strlen(slirp->tftp_server_name);
I think we should check somewhere that val < 256, otherwise you could
confuse the guest and maybe also crash QEMU by specifying a string that
is too big.
Same applies for the hostname and domainname parameter, though, so the
problem is already pre-existing, e.g.:
$ qemu-system-x86_64 -nic user,hostname=`for ((x=0;x<6000;x++)); do \
echo -n y ; done`,domainname=`for ((x=0;x<6000;x++)); do \
echo -n z ; done`
*** Error in `qemu-system-x86_64': malloc(): memory corruption:
0x00007f6c101a50e0 ***
There already seems to be a sanity check in net_slirp_init() that the
domainname is not empty, so I guess that would be a good place to check
that the hostname, domainname and tftp_server_name lengths are smaller
than 256? Anyway, could also be a patch on top, but we really should not
forget about this.
> + *q++ = RFC2132_TFTP_SERVER_NAME;
> + *q++ = val;
> + memcpy(q, slirp->tftp_server_name, val);
> + q += val;
> + }
> +
> if (slirp->vdnssearch) {
> size_t spaceleft = sizeof(rbp->bp_vend) - (q - rbp->bp_vend);
> val = slirp->vdnssearch_len;
> diff --git a/slirp/bootp.h b/slirp/bootp.h
> index 394525733e..4043489835 100644
> --- a/slirp/bootp.h
> +++ b/slirp/bootp.h
> @@ -70,6 +70,7 @@
> #define RFC2132_MAX_SIZE 57
> #define RFC2132_RENEWAL_TIME 58
> #define RFC2132_REBIND_TIME 59
> +#define RFC2132_TFTP_SERVER_NAME 66
>
> #define DHCPDISCOVER 1
> #define DHCPOFFER 2
> diff --git a/slirp/libslirp.h b/slirp/libslirp.h
> index 740408a96e..0e84ab9b52 100644
> --- a/slirp/libslirp.h
> +++ b/slirp/libslirp.h
> @@ -13,10 +13,12 @@ Slirp *slirp_init(int restricted, bool in_enabled, struct
> in_addr vnetwork,
> bool in6_enabled,
> struct in6_addr vprefix_addr6, uint8_t vprefix_len,
> struct in6_addr vhost6, const char *vhostname,
> + const char *tftp_server_name,
> const char *tftp_path, const char *bootfile,
> struct in_addr vdhcp_start, struct in_addr vnameserver,
> struct in6_addr vnameserver6, const char **vdnssearch,
> - const char *vdomainname, void *opaque);
> + const char *vdomainname,
> + void *opaque);
No need to move opaque to a separate line here?
> void slirp_cleanup(Slirp *slirp);
>
> void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout);
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 5c3bd6163f..4073b0174f 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -283,10 +283,12 @@ Slirp *slirp_init(int restricted, bool in_enabled,
> struct in_addr vnetwork,
> bool in6_enabled,
> struct in6_addr vprefix_addr6, uint8_t vprefix_len,
> struct in6_addr vhost6, const char *vhostname,
> + const char *tftp_server_name,
> const char *tftp_path, const char *bootfile,
> struct in_addr vdhcp_start, struct in_addr vnameserver,
> struct in6_addr vnameserver6, const char **vdnssearch,
> - const char *vdomainname, void *opaque)
> + const char *vdomainname,
> + void *opaque)
dito
> {
> Slirp *slirp = g_malloc0(sizeof(Slirp));
>
> @@ -321,6 +323,7 @@ Slirp *slirp_init(int restricted, bool in_enabled, struct
> in_addr vnetwork,
> slirp->vdhcp_startaddr = vdhcp_start;
> slirp->vnameserver_addr = vnameserver;
> slirp->vnameserver_addr6 = vnameserver6;
> + slirp->tftp_server_name = g_strdup(tftp_server_name);
>
> if (vdnssearch) {
> translate_dnssearch(slirp, vdnssearch);
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index 10b410898a..b80725a0d6 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -212,6 +212,7 @@ struct Slirp {
> /* tftp states */
> char *tftp_prefix;
> struct tftp_session tftp_sessions[TFTP_SESSIONS_MAX];
> + char *tftp_server_name;
>
> ArpTable arp_table;
> NdpTable ndp_table;
>
Thomas