On 01/09/2025 10:22, Kaushlendra Kumar wrote:
> The current check "if (n == sizeof(buf))" is incorrect for detecting
> buffer overflow from readlink(). When readlink() fills the entire
> buffer, it returns sizeof(buf) but does not null-terminate the string,
> leading to potential buffer overrun in subsequent string operations.
>
> Fix by changing the condition to "n >= sizeof(buf)" to properly detect
> when the buffer is completely filled, ensuring space is reserved for
> null termination.
>
> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@xxxxxxxxx>
> ---
> tools/bpf/bpftool/common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index b07317d2842f..eebaa6896bd1 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -464,7 +464,7 @@ int get_fd_type(int fd)
> p_err("can't read link type: %s", strerror(errno));
> return -1;
> }
> - if (n == sizeof(buf)) {
> + if (n >= sizeof(buf)) {
> p_err("can't read link type: path too long!");
> return -1;
> }
Hi and thanks, but I don't understand the change. On success, readlink()
returns the number of bytes placed in the buffer, which is at most
sizeof(buf) in our case, given that it silently truncates the string if
the buffer is too small. So we can never have "n > sizeof(buf)" here?
The current code looks correct to me.
Did you actually hit the buffer overflow you describe?
Quentin
