[PATCH] tftp: make TFTP ports unconditionally configurable

[Quentin Schulz]

On 10/6/25 4:22 PM, Quentin Schulz wrote:
> Hi Alvin,
> 
> On 10/2/25 11:43 AM, Alvin Šipraga wrote:
>> [You don't often get email from alvin at pqrs.dk. Learn why this is 
>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> A few lines of code being guarded by the CONFIG_TFTP_PORT option seems
>> an unnecessary restriction on the TFTP support provided by a vanilla
>> U-Boot image. In cases where the TFTP server cannot run as superuser -
>> and hence cannot run on the well-known port 69 - this quirk incurs a
>> full reconfiguration and rebuild of the bootloader only in order to
>> select the appropriate destination port.
>>
>> Remove the CONFIG_TFTP_PORT option entirely and make the tftpdstp and
>> tftpsrcp variables always have an effect. Their being unset will mean
>> that U-Boot behaves the same as if CONFIG_TFTP_PORT was unset. Update
>> the documentation accordingly. And fix up the single board which was
>> originally enabling this option.
>>
>> Signed-off-by: Alvin Šipraga <alvin at pqrs.dk>
>> ---
>> ---
>>   configs/gurnard_defconfig |  1 -
>>   doc/usage/cmd/tftpput.rst |  8 ++------
>>   net/Kconfig               | 18 ------------------
>>   net/tftp.c                |  3 +--
>>   4 files changed, 3 insertions(+), 27 deletions(-)
>>
>> diff --git a/configs/gurnard_defconfig b/configs/gurnard_defconfig
>> index 
>> bde531a6003a2d7167767b3f509876137c7da1a8..85643f249aed65071005a157658c3dc11f44c619 100644
>> --- a/configs/gurnard_defconfig
>> +++ b/configs/gurnard_defconfig
>> @@ -42,7 +42,6 @@ CONFIG_ENV_OVERWRITE=y
>>   CONFIG_ENV_IS_IN_NAND=y
>>   CONFIG_ENV_RELOC_GD_ENV_ADDR=y
>>   CONFIG_NET_RETRY_COUNT=20
>> -CONFIG_TFTP_PORT=y
>>   CONFIG_TFTP_TSIZE=y
>>   CONFIG_AT91_GPIO=y
>>   CONFIG_GENERIC_ATMEL_MCI=y
>> diff --git a/doc/usage/cmd/tftpput.rst b/doc/usage/cmd/tftpput.rst
>> index 
>> 351c9faa38b9d8357c0e200d8cebd1f90344e0a6..2bcb3032cb2444926da28454eefdb2b128a7389f 100644
>> --- a/doc/usage/cmd/tftpput.rst
>> +++ b/doc/usage/cmd/tftpput.rst
>> @@ -19,9 +19,8 @@ Description
>>   The tftpput command is used to transfer a file to a TFTP server.
>>
>>   By default the destination port is 69 and the source port is pseudo- 
>> random.
>> -If CONFIG_TFTP_PORT=y, the environment variable *tftpsrcp* can be 
>> used to set
>> -the source port and the environment variable *tftpdstp* can be used 
>> to set
>> -the destination port.
>> +The environment variable *tftpsrcp* can be used to set the source 
>> port and the
>> +environment variable *tftpdstp* can be used to set the destination port.
>>
>>   address
>>       memory address where the data starts
>> @@ -75,9 +74,6 @@ The command is only available if CONFIG_CMD_TFTPPUT=y.
>>   CONFIG_TFTP_BLOCKSIZE defines the size of the TFTP blocks sent. It 
>> defaults
>>   to 1468 matching an ethernet MTU of 1500.
>>
>> -If CONFIG_TFTP_PORT=y, the environment variables *tftpsrcp* and 
>> *tftpdstp* can
>> -be used to set the source and the destination ports.
>> -
>>   CONFIG_TFTP_WINDOWSIZE can be used to set the TFTP window size of 
>> transmits
>>   after which an ACK response is required. The window size defaults to 1.
>>
>> diff --git a/net/Kconfig b/net/Kconfig
>> index 
>> 40ec6bbce7614afde5a2a9f90d431f19a3654a41..7ba64d43b398415825921e7b61dc4c81e986618a 100644
>> --- a/net/Kconfig
>> +++ b/net/Kconfig
>> @@ -60,24 +60,6 @@ config SYS_FAULT_ECHO_LINK_DOWN
>>            this option is active, then CONFIG_SYS_FAULT_MII_ADDR also 
>> needs to
>>            be configured.
>>
>> -config TFTP_PORT
>> -       bool "Set TFTP UDP source/destination ports via the environment"
>> -       help
>> -         If this is defined, the environment variable tftpsrcp is 
>> used to
>> -         supply the TFTP UDP source port value.  If tftpsrcp isn't 
>> defined,
>> -         the normal pseudo-random port number generator is used.
>> -
>> -         Also, the environment variable tftpdstp is used to supply 
>> the TFTP
>> -         UDP destination port value.  If tftpdstp isn't defined, the 
>> normal
>> -         port 69 is used.
>> -
>> -         The purpose for tftpsrcp is to allow a TFTP server to 
>> blindly start
>> -         the TFTP transfer using the pre-configured target IP address 
>> and UDP
>> -         port. This has the effect of "punching through" the (Windows 
>> XP)
>> -         firewall, allowing the remainder of the TFTP transfer to 
>> proceed
>> -         normally.  A better solution is to properly configure the 
>> firewall,
>> -         but sometimes that is not allowed.
>> -
>>   config TFTP_WINDOWSIZE
>>          int "TFTP window size"
>>          default 1
>> diff --git a/net/tftp.c b/net/tftp.c
>> index 
>> 1ca9a5ea7cf6bac562006e2506ebcb17996a8c36..1760877107faef57107cb04c484157750ff63602 100644
>> --- a/net/tftp.c
>> +++ b/net/tftp.c
>> @@ -926,14 +926,13 @@ void tftp_start(enum proto_t protocol)
>>          /* Use a pseudo-random port unless a specific port is set */
>>          tftp_our_port = 1024 + (get_timer(0) % 3072);
>>
>> -#ifdef CONFIG_TFTP_PORT
>>          ep = env_get("tftpdstp");
>>          if (ep != NULL)
>>                  tftp_remote_port = simple_strtol(ep, NULL, 10);
>>          ep = env_get("tftpsrcp");
>>          if (ep != NULL)
>>                  tftp_our_port = simple_strtol(ep, NULL, 10);
>> -#endif
>> +
> 
> The only worry I have with this patch is the security implications of 
> that. Indeed, if I manage to get access to the host (root not required) 
> where the TFTP server is running on port 69 AND the environment of the 
> board, I could set tftpdstp to point to the TFTP server I (an attacker) 
> just started on a non-privileged port.
> 

Thinking about this five more seconds, I guess at that point, one could 
replace the command calling tftp directly and point U-Boot to a 
different IP for the TFTP server or do other nastier things.

If you're blindly trusting the environment from an external source, it's 
your problem anyway, simply allow importing some variables to be 
imported from other environments and limit the exposure that way.

So I guess it's fine as is for this patch,

Reviewed-by: Quentin Schulz <quentin.schulz at cherry.de>

Thanks!
Quentin