From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f176.google.com (mail-pg1-f176.google.com [209.85.215.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 801A226CE11 for ; Mon, 4 May 2026 05:55:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.176 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777874157; cv=none; b=GpOM88grq1y3I1BIAu2qgze/UXLZW29Gjxv9k9j3LHlWxSFJXOBbXasUD5yPJN7OG02N90FMNqD7EecmTDr6x3q9aUXcEOElbqPHEKRDGI72VqcO11N/WFqyixFws17+a7BTbO8r/olaDv7KJ4Uh+evXs0sJgsuyyp98re+vjYc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777874157; c=relaxed/simple; bh=0JYb+47SuTUvKMIjNtlWIKxuIpEYewG2doFKKx6vhiE=; h=Message-ID:Date:MIME-Version:From:Subject:To:Cc:References: In-Reply-To:Content-Type; b=py5HwdT+RIjXv6SlLmuTF4AWrE37q4RON80dSk1QBEWFOt1Oon9YOh/K2Ybrn3eoMTIQTjSfkl2dcVPtDbkHuKoy//JD36NnT8RC7/7Gmu3Jr5ytWzu5IAXsq14ToE73ndF5uZqjlr/DyJzAmXCJq3JQbuI5Qr5gHFivFfQEXvI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=cpeGvX2V; arc=none smtp.client-ip=209.85.215.176 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="cpeGvX2V" Received: by mail-pg1-f176.google.com with SMTP id 41be03b00d2f7-c80203b9d7bso247416a12.0 for ; Sun, 03 May 2026 22:55:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1777874156; x=1778478956; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=qSbGg0k0BvR8bHpFwu2SlmO/vZ7U24qUHDBvD74bJMk=; b=cpeGvX2VD+eKwqRK/mTAQSu7JYqkRVOgDCHPK7JH/eHxK6YfwTqSONZhoGh8J6G2sL d1pJ7HtPRF2NpKOvB6dM5bvdyhPOT6FeXCnfD0ZhfZTRJJKcSxMxRuJyhg+UqOT6a7SE 5PM9aIUYDLmrPnJlvrD9veY2YdPEsR2ohRPSIygOVqzbwwEVF4KzaiCa8Z6/Bdt1IPJb WZ1SGmedNImDOdcV6ZtMSiNYv4E20zc8getbujpVSNyZuUcg4D/EeVwvvfTnOVxHB8vU R/652r/kbzLE4hpkAWjTLK9aRxwKOzmBC97xAkKmo7d60tzzd0fDeWny47Tpm7Yv4WM2 9mDg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1777874156; x=1778478956; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=qSbGg0k0BvR8bHpFwu2SlmO/vZ7U24qUHDBvD74bJMk=; b=qZQKUftBdJwbYRW8hVyogdbchc2oYA6N6B0YETZyF4g318T4fyn7F6wZ+Jf/3t9FHW u2muP5DnmlgQePzkw98LlQKSo+Q0HEJO/VoHJuHu5pRznA21MlFtRkRpgasecH3o33oO cQGLMeuSgkFAdQu9DqkoYCFuANxcJumzeSSdR36v9de4Ntg3wEQjDgQBqRYF8h4VWXFi HwwAl6DqEnw9IyWSVrC0oj5qTmL6b7ISIGSQsRVVKPlLwVWtormpRQonp1QFeln1NBMH xc3jbWvmA5wOGBzx/5YYvMedIHUrPrSedw9yEOiBrAQLaYDuZNK/orb+5OvTAb0VKNJI skuA== X-Forwarded-Encrypted: i=1; AFNElJ+P4N39jongYcEGdjXEuKgGrYUPD7JT//Lk6kFt9WTTHjgLAcmc30A/1qiXU92C+mOglpfqvrlQpjAhahE=@vger.kernel.org X-Gm-Message-State: AOJu0Yz6CsRuf60WrTf+rYJ8eyScBBGDof3lm+0azOcsQYWgYCSG3G83 /7ipDs8gD1TZG3P1Nokda9G8cXpIdkqQK3mfB0smctvGbgSKtj3FPP6F X-Gm-Gg: AeBDiesgkjSUnDwOKLlFCgyjIeCGqoq7NRJ1RvfNZC4DV1UJX67B7wolBp5cDqOzdWr dXEJKiCgHeoFazeq1FrDDvhUfjNBSXUANIXQWYS5BWKmxHOt1dgpMJoCYXceU6BpOQb45zoHBow mkADUqecgYf4wPBD2CO9qMcVxng9ABnKiTiXSr2ZiNr8qee3UUzPKstu78Oy06Tch56NTrMjJWU YyUvi/wWNctLKyWKBbGO0h8JeaCphTCfM1Da4vbNtRmo5LZuLxFeUbzSh0BVTSQjxHqelAx2YBp sWAPPvlWfvzsMgttAQZ/UYuY9Kp1kqo9+UOaaE8Svju3T/18VHnG2zJnpwOiN89X7Xip09M4n3y KccGEejXeg/0j5BNJropErQsiURv9bOnW16wpJ03qJN0fvZsOndVY30xB7JblxknAxLI1NsX5+v 6s8oKqa1xh/t3XFcaY5OgxgmUtAK4x/xNwEy6Wp2OujcsQexDZkzkSMlRIkVfvcyDYcLlSYD2z0 i+B X-Received: by 2002:a05:6a20:9392:b0:3a3:a5cd:560d with SMTP id adf61e73a8af0-3a7f1a1df35mr8814589637.9.1777874155812; Sun, 03 May 2026 22:55:55 -0700 (PDT) Received: from ?IPV6:2402:f000:2:4001:944:3690:ccf6:2932? ([2402:f000:2:4001:944:3690:ccf6:2932]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-835158bd62fsm9894938b3a.24.2026.05.03.22.55.54 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 03 May 2026 22:55:55 -0700 (PDT) Message-ID: <75dda615-d4c3-4e14-8c5b-736a42fe4442@gmail.com> Date: Mon, 4 May 2026 13:55:52 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: 007 Subject: Re: [PATCH] tools: gpio: fix buffer overflow and add bounds check To: Maxwell Doose , linux-gpio@vger.kernel.org Cc: brgl@kernel.org, warthog618@gmail.com, linux-kernel@vger.kernel.org References: <20260503190016.13439-1-zxl434815272@gmail.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi Maxwell, > This looks like a patch that could be split, since you're adding > NULL pointer checking, a potential overflow fix, and a style fix all > into one. I like the idea, but we need to split functional changes. Thanks for the review. I agree that these changes should be split into smaller atomic patches. I will prepare a v2 series. > First off, this can be split into a different patch. Secondly, I feel as > if we should make two if statements for this, one for the NULL and 0 > checking and the other for checking num_lines. And returning -EINVAL > will be ambiguous for the caller, so maybe change that. Got it. I will split the argument validation and the num_lines bounds check, and reconsider the return values. > Another thing that can be split. I will drop the O_RDONLY change from v2 unless it is useful as a separate cleanup. > This seems like a stray change. Thanks, I will remove unrelated whitespace changes. > We already invented a solution for this in the form of strscpy(), so > please change this to use that instead Agreed. I will use strscpy() in v2. > Another thing that could be split. I will split the ioctl name fix into a separate patch. Thanks, Zhang Xiaolei On 5/4/26 04:56, Maxwell Doose wrote: > On Sun May 3, 2026 at 2:00 PM CDT, Zhang Xiaolei wrote: >> Replace strcpy() with strncpy() to avoid potential buffer overflow >> in req.consumer. Also add validation for num_lines to prevent >> out-of-bounds access to req.offsets. >> >> Fix incorrect ioctl name in error message. >> >> Signed-off-by: Zhang Xiaolei >> --- >> tools/gpio/gpio-utils.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> > This looks like a patch that could be split, since you're adding > NULL pointer checking, a potential overflow fix, and a style fix all > into one. I like the idea, but we need to split functional changes. > >> diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c >> index 4096bcd511d1..1afd9dff2bed 100644 >> --- a/tools/gpio/gpio-utils.c >> +++ b/tools/gpio/gpio-utils.c >> @@ -65,11 +65,15 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines, >> int i; >> int ret; >> >> + if (!device_name || !lines || !config || !consumer || >> + num_lines == 0 || num_lines > GPIO_V2_LINES_MAX) >> + return -EINVAL; >> + >> > First off, this can be split into a different patch. Secondly, I feel as > if we should make two if statements for this, one for the NULL and 0 > checking and the other for checking num_lines. And returning -EINVAL > will be ambiguous for the caller, so maybe change that. > >> ret = asprintf(&chrdev_name, "/dev/%s", device_name); >> if (ret < 0) >> return -ENOMEM; >> >> - fd = open(chrdev_name, 0); >> + fd = open(chrdev_name, O_RDONLY); >> > Another thing that can be split. > >> if (fd == -1) { >> ret = -errno; >> fprintf(stderr, "Failed to open %s, %s\n", >> @@ -78,27 +82,29 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines, >> } >> >> memset(&req, 0, sizeof(req)); >> + >> > This seems like a stray change. > >> for (i = 0; i < num_lines; i++) >> req.offsets[i] = lines[i]; >> >> req.config = *config; >> - strcpy(req.consumer, consumer); >> + strncpy(req.consumer, consumer, sizeof(req.consumer) - 1); >> + req.consumer[sizeof(req.consumer) - 1] = '\0'; >> > We already invented a solution for this in the form of strscpy(), so > please change this to use that instead, something like: > > strscpy(req.consumer, consumer, sizeof(req.consumer)); > >> req.num_lines = num_lines; >> >> ret = ioctl(fd, GPIO_V2_GET_LINE_IOCTL, &req); >> if (ret == -1) { >> ret = -errno; >> fprintf(stderr, "Failed to issue %s (%d), %s\n", >> - "GPIO_GET_LINE_IOCTL", ret, strerror(errno)); >> + "GPIO_V2_GET_LINE_IOCTL", ret, strerror(errno)); >> > Another thing that could be split. > >> } >> >> if (close(fd) == -1) >> perror("Failed to close GPIO character device file"); >> + >> > Might be another stray change. > >> exit_free_name: >> free(chrdev_name); >> return ret < 0 ? ret : req.fd; >> } >> - >> > Maybe another stray change? > >> /** >> * gpiotools_set_values() - Set the value of gpio(s) >> * @fd: The fd returned by > I like the idea, but we should be splitting some of these changes to > follow the atomic commits idea of the kernel. > > best regards, > maxwell