Commit graph

182 commits

Author SHA1 Message Date
Stefano Brivio
b06014a6b2 tcp: Pass union tcp_conn pointer to destroy and splice timer functions
The pointers are actually the same, but we later pass the container
union to tcp_table_compact(), which might zero the size of the whole
union, and this confuses Coverity Scan.

Given that we have pointers to the container union to start with,
just pass those instead, all the way down to tcp_table_compact().

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
2022-11-25 01:37:36 +01:00
David Gibson
8e914238b6 tcp: Use dual stack sockets for port forwarding when possible
Platforms like Linux allow IPv6 sockets to listen for IPv4 connections as
well as native IPv6 connections.  By doing this we halve the number of
listening sockets we need for TCP (assuming passt/pasta is listening on the
same ports for IPv4 and IPv6).  When forwarding many ports (e.g. -t all)
this can significantly reduce the amount of kernel memory that passt
consumes.

When forwarding all TCP and UDP ports for both IPv4 and IPv6 (-t all
-u all), this reduces kernel memory usage from ~677MiB to ~487MiB
(kernel version 6.0.8 on Fedora 37, x86_64).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:36:04 +01:00
David Gibson
ebf56c7b56 tcp: Consolidate tcp_sock_init[46]
Previous cleanups mean that tcp_sock_init4() and tcp_sock_init6() are
almost identical, and the remaining differences can be easily
parameterized.  Combine both into a single tcp_sock_init_af() function.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:55 +01:00
David Gibson
bb6bb5649d tcp: NAT IPv4-mapped IPv6 addresses like IPv4 addresses
passt usually doesn't NAT, but it does do so for the remapping of the
gateway address to refer to the host.  Currently we perform this NAT with
slightly different rules on both IPv4 addresses and IPv6 addresses, but not
on IPv4-mapped IPv6 addresses.  This means we won't correctly handle the
case of an IPv4 connection over an IPv6 socket, which is possible on Linux
(and probably other platforms).

Refactor tcp_conn_from_sock() to perform the NAT after converting either
address family into an inany_addr, so IPv4 and and IPv4-mapped addresses
have the same representation.

With two new helpers this lets us remove the IPv4 and IPv6 specific paths
from tcp_conn_from_sock().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:51 +01:00
David Gibson
034fa8a58d tcp: Remove v6 flag from tcp_epoll_ref
This bit in the TCP specific epoll reference indicates whether the
connection is IPv6 or IPv4.  However the sites which refer to it are
already calling accept() which (optionally) returns an address for the
remote end of the connection.  We can use the sa_family field in that
address to determine the connection type independent of the epoll
reference.

This does have a cost: for the spliced case, it means we now need to get
that address from accept() which introduces an extran copy_to_user().
However, in future we want to allow handling IPv4 connectons through IPv6
sockets, which means we won't be able to determine the IP version at the
time we create the listening socket and epoll reference.  So, at some point
we'll have to pay this cost anyway.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:48 +01:00
David Gibson
7f1f2f3f51 tcp: Fix small errors in tcp_seq_init() time handling
It looks like tcp_seq_init() is supposed to advance the sequence number
by one every 32ns.  However we only right shift the ns part of the timespec
not the seconds part, meaning that we'll advance by an extra 32 steps on
each second.

I don't know if that's exploitable in any way, but it doesn't appear to be
the intent, nor what RFC 6528 suggests.

In addition, we convert from seconds to nanoseconds with a multiplication
by '1E9'.  In C '1E9' is a floating point constant, forcing a conversion
to floating point and back for what should be an integer calculation
(confirmed with objdump and Makefile default compiler flags).  Spell out
1000000000 in full to avoid that.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:45 +01:00
David Gibson
e7bae5bafd tcp: Have tcp_seq_init() take its parameters from struct tcp_conn
tcp_seq_init() takes a number of parameters for the connection, but at
every call site, these are already populated in the tcp_conn structure.
Likewise we always store the result into the @seq_to_tap field.

Use this to simplify tcp_seq_init().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:43 +01:00
David Gibson
a93bfada5b tcp: Unify initial sequence number calculation for IPv4 and IPv6
tcp_seq_init() has separate paths for IPv4 and IPv6 addresses, which means
we will calculate different sequence numbers for IPv4 and equivalent
IPv4-mapped IPv6 addresses.

Change it to treat these the same by always converting the input address
into an inany_addr representation and use that to calculate the sequence
number.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:41 +01:00
David Gibson
7114fc92eb tcp: Simplify tcp_hash_match() to take an inany_addr
tcp_hash_match() can take either an IPv4 (struct in_addr) or IPv6 (struct
in6_addr) address.  It has two different paths for each of those cases.
However, its only caller has already constructed an equivalent inany
representation of the address, so we can have tcp_hash_match take that
directly and use a simpler comparison with the inany_equals() helper.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:39 +01:00
David Gibson
eb050b5a19 tcp: Take tcp_hash_insert() address from struct tcp_conn
tcp_hash_insert() takes an address to control which hash bucket the
connection will go into.  However, an inany_addr representation of that
address is already stored in struct tcp_conn.

Now that we've made the hashing of IPv4 and IPv4-mapped IPv6 addresses
equivalent, we can simplify tcp_hash_insert() to use the address in
struct tcp_conn, rather than taking it as an extra parameter.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:36 +01:00
David Gibson
685f97b8de tcp: Hash IPv4 and IPv4-mapped-IPv6 addresses the same
In the tcp_conn structure, we represent the address with an inany_addr
which could be an IPv4 or IPv6 address.  However, we have different paths
which will calculate different hashes for IPv4 and equivalent IPv4-mapped
IPv6 addresses.  This will cause problems for some future changes.

Make the hash function work the same for these two cases, by taking an
inany_addr directly.  Since this represents IPv4 and IPv4-mapped IPv6
addresses the same way, it will trivially hash the same for both cases.

Callers are changed to construct an inany_addr from whatever they have.
Some of that will be elided in later changes.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:34 +01:00
David Gibson
ca69c3f196 inany: Helper functions for handling addresses which could be IPv4 or IPv6
struct tcp_conn stores an address which could be IPv6 or IPv4 using a
union.  We can do this without an additional tag by encoding IPv4 addresses
as IPv4-mapped IPv6 addresses.

This approach is useful wider than the specific place in tcp_conn, so
expose a new 'union inany_addr' like this from a new inany.h.  Along with
that create a number of helper functions to make working with these "inany"
addresses easier.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:32 +01:00
David Gibson
b65d603e23 tcp: Don't store hash bucket in connection structures
Currently when we insert a connection into the hash table, we store its
bucket number so we can find it when removing entries.  However, we can
recompute the hash value from other contents of the structure so we don't
need to store it.  This brings the size of tcp_tap_conn down to 64 bytes
again, which means it will fit in a single cacheline on common machines.

This change also removes a non-obvious constraint that the hash table have
less than twice TCP_MAX_CONNS buckets, because of the way
TCP_HASH_BUCKET_BITS was constructed.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:28 +01:00
David Gibson
233b95e90f tcp: Remove splice from tcp_epoll_ref
Currently the epoll reference for tcp sockets includes a bit indicating
whether the socket maps to a spliced connection.  However, the reference
also has the index of the connection structure which also indicates whether
it is spliced.  We can therefore avoid the splice bit in the epoll_ref by
unifying the first part of the non-spliced and spliced handlers where we
look up the connection state.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:25 +01:00
David Gibson
d909fda1e8 tcp: Use the same sockets to listen for spliced and non-spliced connections
In pasta mode, tcp_sock_init[46]() create separate sockets to listen for
spliced connections (these are bound to localhost) and non-spliced
connections (these are bound to the host address).  This introduces a
subtle behavioural difference between pasta and passt: by default, pasta
will listen only on a single host address, whereas passt will listen on
all addresses (0.0.0.0 or ::).  This also prevents us using some additional
optimizations that only work with the unspecified (0.0.0.0 or ::) address.

However, it turns out we don't need to do this.  We can splice a connection
if and only if it originates from the loopback address.  Currently we
ensure this by having the "spliced" listening sockets listening only on
loopback.  Instead, defer the decision about whether to splice a connection
until after accept(), by checking if the connection was made from the
loopback address.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:22 +01:00
David Gibson
356c6e0677 tcp: Unify part of spliced and non-spliced conn_from_sock path
In tcp_sock_handler() we split off to handle spliced sockets before
checking anything else.  However the first steps of the "new connection"
path for each case are the same: allocate a connection entry and accept()
the connection.

Remove this duplication by making tcp_conn_from_sock() handle both spliced
and non-spliced cases, with help from more specific tcp_tap_conn_from_sock
and tcp_splice_conn_from_sock functions for the later stages which differ.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:19 +01:00
David Gibson
73d3a3e84e tcp: Separate helpers to create ns listening sockets
tcp_sock_init*() can create either sockets listening on the host, or in
the pasta network namespace (with @ns==1).  There are, however, a number
of differences in how these two cases work in practice though.  "ns"
sockets are only used in pasta mode, and they always lead to spliced
connections only.  The functions are also only ever called in "ns" mode
with a NULL address and interface name, and it doesn't really make sense
for them to be called any other way.

Later changes will introduce further differences in behaviour between these
two cases, so it makes more sense to use separate functions for creating
the ns listening sockets than the regular external/host listening sockets.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:02 +01:00
David Gibson
433604a581 tcp: Unify the IN_EPOLL flag
There is very little common between the tcp_tap_conn and tcp_splice_conn
structures.  However, both do have an IN_EPOLL flag which has the same
meaning in each case, though it's stored in a different location.

Simplify things slightly by moving this bit into the common header of the
two structures.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:35:00 +01:00
David Gibson
34476511f7 tcp: Partially unify tcp_timer() and tcp_splice_timer()
These two functions scan all the non-splced and spliced connections
respectively and perform timed updates on them.  Avoid scanning the now
unified table twice, by having tcp_timer scan it once calling the
relevant per-connection function for each one.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:34:58 +01:00
David Gibson
0eef48c4be tcp: Unify tcp_defer_handler and tcp_splice_defer_handler()
These two functions each step through non-spliced and spliced connections
respectively and clean up entries for closed connections.  To avoid
scanning the connection table twice, we merge these into a single function
which scans the unified table and performs the appropriate sort of cleanup
action on each one.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:34:54 +01:00
David Gibson
ee8f8e9564 tcp: Unify spliced and non-spliced connection tables
Currently spliced and non-spliced connections are stored in completely
separate tables, so there are completely independent limits on the number
of spliced and non-spliced connections.  This is a bit counter-intuitive.

More importantly, the fact that the tables are separate prevents us from
unifying some other logic between the two cases.  So, merge these two
tables into one, using the 'c.spliced' common field to distinguish between
them when necessary.

For now we keep a common limit of 128k connections, whether they're spliced
or non-spliced, which means we save memory overall.  If necessary we could
increase this to a 256k or higher total, which would cost memory but give
some more flexibility.

For now, the code paths which need to step through all extant connections
are still separate for the two cases, just skipping over entries which
aren't for them.  We'll improve that in later patches.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:34:51 +01:00
David Gibson
181ce83d9b tcp: Improved helpers to update connections after moving
When we compact the connection tables (both spliced and non-spliced) we
need to move entries from one slot to another.  That requires some updates
in the entries themselves.  Add helpers to make all the necessary updates
for the spliced and non-spliced cases.  This will simplify later cleanups.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:34:48 +01:00
David Gibson
ff27fd63cd tcp: Add connection union type
Currently, the tables for spliced and non-spliced connections are entirely
separate, with different types in different arrays.  We want to unify them.
As a first step, create a union type which can represent either a spliced
or non-spliced connection.  For them to be distinguishable, the individual
types need to have a common header added, with a bit indicating which type
this structure is.

This comes at the cost of increasing the size of tcp_tap_conn to over one
(64 byte) cacheline.  This isn't ideal, but it makes things simpler for now
and we'll re-optimize this later.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:34:46 +01:00
David Gibson
3cf027bd59 tcp: Move connection state structures into a shared header
Currently spliced and non-spliced connections use completely independent
tracking structures.  We want to unify these, so as a preliminary step move
the definitions for both variants into a new tcp_conn.h header, shared by
tcp.c and tcp_splice.c.

This requires renaming some #defines with the same name but different
meanings between the two cases.  In the process we correct some places that
are slightly out of sync between the comments and the code for various
event bit names.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:34:43 +01:00
David Gibson
60be7438fa tcp: Better helpers for converting between connection pointer and index
The macro CONN_OR_NULL() is used to look up connections by index with
bounds checking.  Replace it with an inline function, which means:
    - Better type checking
    - No danger of multiple evaluation of an @index with side effects

Also add a helper to perform the reverse translation: from connection
pointer to index.  Introduce a macro for this which will make later
cleanups easier and safer.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-25 01:34:37 +01:00
Stefano Brivio
817eedc28a tcp, udp: Don't initialise IPv6/IPv4 sockets if IPv4/IPv6 are not enabled
If we disable a given IP version automatically (no corresponding
default route on host) or administratively (--ipv4-only or
--ipv6-only options), we don't initialise related buffers and
services (DHCP for IPv4, NDP and DHCPv6 for IPv6). The "tap"
handlers will also ignore packets with a disabled IP version.

However, in commit 3c6ae62510 ("conf, tcp, udp: Allow address
specification for forwarded ports") I happily changed socket
initialisation functions to take AF_UNSPEC meaning "any enabled
IP version", but I forgot to add checks back for the "enabled"
part.

Reported by Paul: on a host without default IPv6 route, but IPv6
enabled, connect, using IPv6, to a port handled by pasta, which
tries to send data to a tap device without initialised buffers
for that IP version and exits because the resulting write() fails.

Simpler way to reproduce: pasta -6 and inbound IPv4 connection, or
pasta -4 and inbound IPv6 connection.

Reported-by: Paul Holzinger <pholzing@redhat.com>
Fixes: 3c6ae62510 ("conf, tcp, udp: Allow address specification for forwarded ports")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
2022-11-10 11:17:50 +01:00
David Gibson
de93acbe70 tcp: Correct function comments for address types
A number of functions describe themselves as taking a pointer to 'sin_addr
or sin6_addr'.  Those are field names, not type names.  Replace them with
the correct type names, in_addr or in6_addr.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-04 12:04:30 +01:00
David Gibson
7c7b68dbe0 Use typing to reduce chances of IPv4 endianness errors
We recently corrected some errors handling the endianness of IPv4
addresses.  These are very easy errors to make since although we mostly
store them in network endianness, we sometimes need to manipulate them in
host endianness.

To reduce the chances of making such mistakes again, change to always using
a (struct in_addr) instead of a bare in_addr_t or uint32_t to store network
endian addresses.  This makes it harder to accidentally do arithmetic or
comparisons on such addresses as if they were host endian.

We introduce a number of IN4_IS_ADDR_*() helpers to make it easier to
directly work with struct in_addr values.  This has the additional benefit
of making the IPv4 and IPv6 paths more visually similar.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-11-04 12:04:24 +01:00
Stefano Brivio
6f3e38cac5 Don't create 'tap' socket for ports that are bound to loopback only
If the user specifies an explicit loopback address for a port
binding, we're going to use that address for the 'tap' socket, and
the same exact address for the 'spliced' socket (because those are,
by definition, only bound to loopback addresses).

This means that the second binding will fail, and, unexpectedly, the
port is forwarded, but via tap device, which means the source address
in the namespace won't be a loopback address.

Make it explicit under which conditions we're creating which kind of
socket, by refactoring tcp_sock_init() into two separate functions
for IPv4 and IPv6 and gathering those conditions at the beginning.

Also, don't create spliced sockets if the user specifies explicitly
a non-loopback address, those are harmless but not desired either.

Fixes: 3c6ae62510 ("conf, tcp, udp: Allow address specification for forwarded ports")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-10-15 02:10:36 +02:00
Stefano Brivio
d0dd0242a6 tcp, tcp_splice: Fix port remapping for inbound, spliced connections
In pasta mode, when we receive a new inbound connection, we need to
select a socket that was created in the namespace to proceed and
connect() it to its final destination.

The existing condition might pick a wrong socket, though, if the
destination port is remapped, because we'll check the bitmap of
inbound ports using the remapped port (stored in the epoll reference)
as index, and not the original port.

Instead of using the port bitmap for this purpose, store this
information in the epoll reference itself, by adding a new 'outbound'
bit, that's set if the listening socket was created the namespace,
and unset otherwise.

Then, use this bit to pick a socket on the right side.

Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Fixes: 33482d5bf2 ("passt: Add PASTA mode, major rework")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
2022-10-15 02:10:36 +02:00
Stefano Brivio
eab9d8d5d6 tcp, tcp_splice: Adjust comments to current meaning of inbound and outbound
For tcp_sock_init_ns(), "inbound" connections used to be the ones
being established toward any listening socket we create, as opposed
to sockets we connect().

Similarly, tcp_splice_new() used to handle "inbound" connections in
the sense that they originated from listening sockets, and they would
in turn cause a connect() on an "outbound" socket.

Since commit 1128fa03fe ("Improve types and names for port
forwarding configuration"), though, inbound connections are more
broadly defined as the ones directed to guest or namepsace, and
outbound the ones originating from there.

Update comments for those two functions.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
2022-10-15 02:10:36 +02:00
Stefano Brivio
c1eff9a3c6 conf, tcp, udp: Allow specification of interface to bind to
Since kernel version 5.7, commit c427bfec18f2 ("net: core: enable
SO_BINDTODEVICE for non-root users"), we can bind sockets to
interfaces, if they haven't been bound yet (as in bind()).

Introduce an optional interface specification for forwarded ports,
prefixed by %, that can be passed together with an address.

Reported use case: running local services that use ports we want
to have externally forwarded:
  https://github.com/containers/podman/issues/14425

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
2022-10-15 02:10:36 +02:00
Stefano Brivio
da152331cf Move logging functions to a new file, log.c
Logging to file is going to add some further complexity that we don't
want to squeeze into util.c.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
2022-10-14 17:38:25 +02:00
David Gibson
1ae95f33cc cppcheck: Suppress NULL pointer warning in tcp_sock_consume()
Recent versions of cppcheck give a warning due to the NULL buffer passed
to recv() in tcp_sock_consume().  Since this apparently works, I assume
it's actually valid, but cppcheck doesn't know that recv() can take a NULL
buffer.  So, use a suppression to get rid of the error.

Also add an unmatchedSuppression suppression since only some cppcheck
versions complain about this.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-09-29 12:22:32 +02:00
David Gibson
e2b7d370d0 cppcheck: Work around false positive NULL pointer dereference error
Some versions of cppcheck could errneously report a NULL pointer deference
inside a sizeof().  This is now fixed in cppcheck upstream[0].  For systems
using an affected version, add a suppression to work around the bug.  Also
add an unmatchedSuppression suppression so the suppression itself doesn't
cause a warning if you *do* have a fixed cppcheck.

[0] https://github.com/danmar/cppcheck/pull/4471

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-09-29 12:22:13 +02:00
David Gibson
d5b80ccc72 Fix widespread off-by-one error dealing with port numbers
Port numbers (for both TCP and UDP) are 16-bit, and so fit exactly into a
'short'.  USHRT_MAX is therefore the maximum port number and this is widely
used in the code.  Unfortunately, a lot of those places don't actually
want the maximum port number (USHRT_MAX == 65535), they want the total
number of ports (65536).  This leads to a number of potentially nasty
consequences:

 * We have buffer overruns on the port_fwd::delta array if we try to use
   port 65535
 * We have similar potential overruns for the tcp_sock_* arrays
 * Interestingly udp_act had the correct size, but we can calculate it in
   a more direct manner
 * We have a logical overrun of the ports bitmap as well, although it will
   just use an unused bit in the last byte so isnt harmful
 * Many loops don't consider port 65535 (which does mitigate some but not
   all of the buffer overruns above)
 * In udp_invert_portmap() we incorrectly compute the reverse port
   translation for return packets

Correct all these by using a new NUM_PORTS defined explicitly for this
purpose.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
2022-09-24 14:48:35 +02:00
David Gibson
3ede07aac9 Treat port numbers as unsigned
Port numbers are unsigned values, but we're storing them in (signed) int
variables in some places.  This isn't actually harmful, because int is
large enough to hold the entire range of ports.  However in places we don't
want to use an in_port_t (usually to avoid overflow on the last iteration
of a loop) it makes more conceptual sense to use an unsigned int. This will
also avoid some problems with later cleanups.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
2022-09-24 14:48:35 +02:00
David Gibson
f5a31ee94c Don't use indirect remap functions for conf_ports()
Now that we've delayed initialization of the UDP specific "reverse" map
until udp_init(), the only difference between the various 'remap' functions
used in conf_ports() is which array they target.  So, simplify by open
coding the logic into conf_ports() with a pointer to the correct mapping
array.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
2022-09-24 14:48:35 +02:00
David Gibson
163dc5f188 Consolidate port forwarding configuration into a common structure
The configuration for how to forward ports in and out of the guest/ns is
divided between several different variables.  For each connect direction
and protocol we have a mode in the udp/tcp context structure, a bitmap
of which ports to forward also in the context structure and an array of
deltas to apply if the outward facing and inward facing port numbers are
different.  This last is a separate global variable, rather than being in
the context structure, for no particular reason.  UDP also requires an
additional array which has the reverse mapping used for return packets.

Consolidate these into a re-used substructure in the context structure.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
2022-09-24 14:48:35 +02:00
David Gibson
1128fa03fe Improve types and names for port forwarding configuration
enum conf_port_type is local to conf.c and is used to track the port
forwarding mode during configuration.  We don't keep it around in the
context structure, however the 'init_detect_ports' and 'ns_detect_ports'
fields in the context are based solely on this.  Rather than changing
encoding, just include the forwarding mode into the context structure.
Move the type definition to a new port_fwd.h, which is kind of trivial at
the moment but will have more stuff later.

While we're there, "conf_port_type" doesn't really convey that this enum is
describing how port forwarding is configured.  Rename it to port_fwd_mode.
The variables (now fields) of this type also have mildly confusing names
since it's not immediately obvious whether 'ns' and 'init' refer to the
source or destination of the packets.  Use "in" (host to guest / init to
ns) and "out" (guest to host / ns to init) instead.

This has the added bonus that we no longer have locals 'udp_init' and
'tcp_init' which shadow global functions.

In addition, add a typedef 'port_fwd_map' for a bitmap of each port number,
which is used in several places.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
2022-09-24 14:48:35 +02:00
David Gibson
16f5586bb8 Make substructures for IPv4 and IPv6 specific context information
The context structure contains a batch of fields specific to IPv4 and to
IPv6 connectivity.  Split those out into a sub-structure.

This allows the conf_ip4() and conf_ip6() functions, which take the
entire context but touch very little of it, to be given more specific
parameters, making it clearer what it affects without stepping through the
code.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
2022-07-30 22:14:07 +02:00
David Gibson
5e12d23acb Separate IPv4 and IPv6 configuration
After recent changes, conf_ip() now has essentially entirely disjoint paths
for IPv4 and IPv6 configuration.  So, it's cleaner to split them out into
different functions conf_ip4() and conf_ip6().

Splitting these out also lets us make the interface a bit nicer, having
them return success or failure directly, rather than manipulating c->v4
and c->v6 to indicate success/failure of the two versions.

Since these functions may also initialize the interface index for each
protocol, it turns out we can then drop c->v4 and c->v6 entirely, replacing
tests on those with tests on whether c->ifi4 or c->ifi6 is non-zero (since
a 0 interface index is never valid).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: Whitespace fixes]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-07-30 22:12:50 +02:00
David Gibson
4b2e018d70 Allow different external interfaces for IPv4 and IPv6 connectivity
It's quite plausible for a host to have both IPv4 and IPv6 connectivity,
but only via different interfaces.  For example, this will happen in the
case that IPv6 connectivity is via a tunnel (e.g. 6in4 or 6rd).  It would
also happen in the case that IPv4 access is via a tunnel on an otherwise
IPv6 only local network, which is a setup that might become more common in
the post IPv4 address exhaustion world.

In turns out there's no real need for passt/pasta to get its IPv4 and IPv6
connectivity via the same interface, so we can handle this situation fairly
easily.  Change the core to allow eparate external interfaces for IPv4 and
IPv6.  We don't actually set these separately for now.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
2022-07-30 21:50:41 +02:00
Stefano Brivio
deda03bfc2 tcp: Silence warning from gcc 11.3 with -Ofast
If the first packet_get() call doesn't assign len, the second one
will also return NULL, but gcc doesn't see this.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-06-08 11:08:29 +02:00
Stefano Brivio
d27cc3e435 tcp: Work around gcc 12 bogus warning in tcp_rtt_dst_check()
gcc 12.1.x (e.g. current OpenSUSE Tumbleweed, x86_64 only,
gcc-12-1.4.x86_64) reports:

tcp.c: In function ‘tcp_send_flag’:
tcp.c:1014:9: warning: writing 16 bytes into a region of size 0 [-Wstringop-overflow=]
 1014 |         memcpy(low_rtt_dst + hole++, &conn->a.a6, sizeof(conn->a.a6));
      |         ^
tcp.c:559:24: note: at offset -16 into destination object ‘low_rtt_dst’ of size 128
  559 | static struct in6_addr low_rtt_dst[LOW_RTT_TABLE_SIZE];
      |

but 'hole' can't be -1, because the low_rtt_dst table is guaranteed
to have a hole: if we happened to write to the last entry, we'll go
back to index 0 and clear that one.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-05-20 10:36:11 +02:00
Stefano Brivio
3c6ae62510 conf, tcp, udp: Allow address specification for forwarded ports
This feature is available in slirp4netns but was missing in passt and
pasta.

Given that we don't do dynamic memory allocation, we need to bind
sockets while parsing port configuration. This means we need to
process all other options first, as they might affect addressing and
IP version support. It also implies a minor rework of how TCP and UDP
implementations bind sockets.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-05-01 07:19:05 +02:00
Stefano Brivio
5ab2e12f98 tcp: False "Out-of-bounds read" positive, CWE-125
Reported by Coverity: it doesn't see that tcp{4,6}_l2_buf_used are
set to zero by tcp_l2_data_buf_flush(), repeat that explicitly here.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-04-07 11:44:35 +02:00
Stefano Brivio
2a3b8dad33 tcp, tcp_splice: False "Negative array index read" positives, CWE-129
A flag or event bit is always set by callers. Reported by Coverity.

Signed-by-off: Stefano Brivio <sbrivio@redhat.com>
2022-04-07 11:44:35 +02:00
Stefano Brivio
71a00f1449 tcp: Dereference null return value, CWE-476
Not an issue with a sane kernel behaviour. Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-04-07 11:44:35 +02:00
Stefano Brivio
22ed4467a4 treewide: Unchecked return value from library, CWE-252
All instances were harmless, but it might be useful to have some
debug messages here and there. Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
2022-04-07 11:44:35 +02:00