From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) (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 253AC18BBAE for ; Mon, 4 May 2026 05:46:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.48 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777873568; cv=none; b=mcY074j8jiS71Akt+dEML+O9lE1ZC12pQiKjU6KhGaKRIsGruczJyNCSCe7JU2EBK56cdf4bY34zq5ywGZ6kOXmbrcURNGhcxIUw+UliVnyE8xX4uoHvbVO5VpdyEF52hjipOv9NTHoWVTUfd/gotB4NUa84pjzY9SU/FFveR3Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777873568; c=relaxed/simple; bh=9JqMPn0RXpz+fcD36D3dFToJCRezEpPBGiispDmrjPk=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=axyKKRZ8vJiB9Q0lDK9QbNh5gEEb0XCA4aOWLMluDI7Fviy4HlQ1ZFBULpktaXANM8K+w3JzWldknXzjni1rItEKf+4ZhAXXMyhJnU9yFX83GfipMLpmzrOy7pxQeulR0O2DQ9IxdsJ5UuHOrNyy8OTsdnwnl3xVap7t8oGTvps= 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=jA0YhjvF; arc=none smtp.client-ip=209.85.216.48 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="jA0YhjvF" Received: by mail-pj1-f48.google.com with SMTP id 98e67ed59e1d1-35fb16e56efso2502338a91.2 for ; Sun, 03 May 2026 22:46:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1777873566; x=1778478366; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=JMdrqXAeZlHZMAjNWM5ngvOGBdh+mR1QfsCscY7ufQo=; b=jA0YhjvF1Ld81pQIpOJeHUPNpOwHVIeJ1r1/t3HX1NJ8UNJQZMVAmp3fFCQPCsEf7b NfqDGuay8vtp8EwO/NYpazjV3Gs7guJihDKai2Ah5K1/D50iujD+7xZgqUWx4N/H1qQM bKU+sf050EcJ5Zu/izSzmmr2eTlEHudcolQmLD3MoTiGRuK5VOfqj4A8zaAg1OGFnUIF aL7DTGLLP7/6VenhDulD3AG/sPB2T/TEPDqJxP4DSm6QJ20HHvXHg93H6IvehWvUop4T fnJuZbdmtjHAoi1WU3GwDGR4obZI2V2H72v5R05/sgul3He+iu/7WVEoIHoDxgn71e7j WhaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1777873566; x=1778478366; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=JMdrqXAeZlHZMAjNWM5ngvOGBdh+mR1QfsCscY7ufQo=; b=pM+y+1yJxz+Rgacvf+BhbPDaAKB1OY+HUR6s01xFnBsVeSBbiRSVkqoGBQ6PzFBgBf jNt6OrjiQl3SEttQAR2JuUfLVnssy1wm17Q4D+iObjH3cUlGTohrejFLoVI0l9MJbinY KY5abv0OxsWe6TGSYWWiyuvJn+9m9CYl4c4f97Mj0E+Y5TBgTy98KCGl2KXO8ho/99Wv eocvTxKsk9NTUpkMsVqfwL3y7dZ8oLFos9JFfX2Kgor65T6smRgz1JjldRKukXzqFrZQ CRUA43Rv9Pt0qp05xM+qM+jdHWQGK0EIz1iCvYrdkbDSJQLUl8zvSPaRaYp6fVj64p0H EpRg== X-Forwarded-Encrypted: i=1; AFNElJ8GSEG1G2zD30hD0lyP0VJWop5C6CbnXplZ+qGix5fnnqcTfPxFN1ccQjCrpR0T0nkO2/BBcWWa3rwdovA=@vger.kernel.org X-Gm-Message-State: AOJu0YwKPcD2mqu9H48q5MhUqMxVAV08bp2CihO/K+2g2y3xzbTYHd8/ FfdzEDT1jbgBqhuY8RNoG+qRY1FSmSIo3lF3r98akyzrN7ZsCEJb13HM X-Gm-Gg: AeBDievHlJKhaeJStRate4CoVHJ5oC9OorDWc+AE1ZRxeSBVGRm2H9L98cTs9PIuxql PDtakeGG102efAke0HUG5KuwrL7fLu76r9jDRbSWQbK3Jx+rcZvO4JjeOw2ppeHLCDKJCSvM9/J lq3QIzAF/bVzVUx1MJNy8jPobU5z/Y5QNc7Go0OTeCy3iG1E1d8Rhim6WacYKM7haLHnsa5yO1a pyh5kxP7hGuM2eMWxV1BGNlGMrj48ipXgswoV/KIch1xk+kaobEg+EbMQ+Cmuq7u02XPVu6ZmF5 dGDIHn1j6Rk+tITrOU5Fu18M50LUy3H4XsjytIGvehQ0nUfV3lcunPaxyVipnLztBNRkhlmdj7J oruX61Bk4cBok23QbJ+7gYQF+3pNACl9B1jP+nLNQDgZ/2C7KU9G6wOfjsPEwDaW+ZM8vkc+CXl z756CzcY4HN8XA3U3eYPTj7Pw4G0O+shEBN52TLWOVuOeC0zqfEbK9Z1NDaY2yznFFBocLoyV02 isqwsg= X-Received: by 2002:a05:6a20:3d84:b0:398:8002:8033 with SMTP id adf61e73a8af0-3a7f1fce0a8mr9056079637.49.1777873566326; Sun, 03 May 2026 22:46:06 -0700 (PDT) Received: from ?IPV6:2402:f000:2:4001:427f:2eb3:870d:a19d? ([2402:f000:2:4001:427f:2eb3:870d:a19d]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-c7ffbc6f063sm8457392a12.20.2026.05.03.22.46.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 03 May 2026 22:46:06 -0700 (PDT) Message-ID: <81958e85-da7b-4b4d-927b-f7515a2b02a3@gmail.com> Date: Mon, 4 May 2026 13:46:02 +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 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 From: 007 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