From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f54.google.com (mail-ot1-f54.google.com [209.85.210.54]) (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 0639C37C0FC for ; Sun, 3 May 2026 20:56:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777841783; cv=none; b=rDHn54Z/qkvcplTly0nPWMd4i3kZZ1NrhhHql+wvAOpiBBKVwV/Wl7UpsL8RHS4h1qOwGercUWdBlMwghs6CU1CL4mMuY35E3GHDPn3NHKbM0pH6y4kP1RP56btRdpcFyjr0fMRTygzti2VAVsMjzwA/umhA7g7vnTS6XkZfKMI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777841783; c=relaxed/simple; bh=X882GCO+Dvlf2ebX1aZh7TkhwiTLTaW9iuH+RMqeYYU=; h=Mime-Version:Content-Type:Date:Message-Id:Subject:From:To:Cc: References:In-Reply-To; b=SYe4F2t8CY6Xm8PrLsdiQnpOOI2E2Z8QXc9l8AkbAwLCpkSiox848q0QKAXQz75SOS+oeY9XddGL0HZo3cvVkB00H+gz4L0OVT2i5s4r3Wzm8Q8Waxsmq9/IpHHOG+qr66UeRSPEo51gr+nEj/RF2+fRHEz0UWN00HUrEuVX/aQ= 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=RCYHiwkg; arc=none smtp.client-ip=209.85.210.54 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="RCYHiwkg" Received: by mail-ot1-f54.google.com with SMTP id 46e09a7af769-7de4a9cb8eeso3134222a34.0 for ; Sun, 03 May 2026 13:56:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1777841780; x=1778446580; darn=vger.kernel.org; h=in-reply-to:references:cc:to:from:subject:message-id:date :content-transfer-encoding:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=s4qRCFUgyMlFXryUo+UrBwsmSJQu18VkeGRzbCwuX5w=; b=RCYHiwkgXaOzIgEKpsfEnRCJKhkVgv7iZuOIrOQEClbxmxCnZ76zMLMc5B8thQaVAG CAYsSLiUsnCUY9R1m0GdLWYRUMCsTOcPww+ju930pjpDtG3ZDKhpETdsm9JJnnqxLWvA dAVIWMqFJOb4aGV79pIL+6pkqKncNVmH9wwCBdyQve65dcGeKF3pqiY64f2ObeaWBkne yySlcLDT5wZZOn1zS2Uwuury7533exEsZXvoMac82T1Al/WUTmnv3SXgubN9bFD20CGo 2LLAHZjYDZyKDqcQEa32Au3KgHt8Dmc+31zRkRR/XC3e/r7/fKbi89mrTDDQCQIeLJN8 X2NA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1777841780; x=1778446580; h=in-reply-to:references:cc:to:from:subject:message-id:date :content-transfer-encoding:mime-version:x-gm-gg:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=s4qRCFUgyMlFXryUo+UrBwsmSJQu18VkeGRzbCwuX5w=; b=MC3ifbgexFQHms3a1nvmf6Z4cV79Z2MOIQ4gA8sRl42K/q7aM4n1IWRNYLXmf5T4hd l6z8POG01R+BY1LiAsG3CrtpZAHQ4LcfKxzApECLRYb3zSiwbVlsy1OGautuoiCKcY1X HYKlrFSZx4t/7oTHEpiTTh9Uy2dU9tga5tk0Sh41TK5sUyrhXx+9MZHDo1zeMJqg3tG3 Csu39wHUPX/F6JIk2pekcrw9YywP3RHPkoszBHRebkL/Ydu4qLSem4n8f9lOBwPZ50t7 V1bEChY6hFWyKnRnxDLGqQtaK9HGhR//oQlBAoluFa7676+VSuMgGYvbW8A+jCqUUvAa t3tA== X-Forwarded-Encrypted: i=1; AFNElJ/0VNxY8/hXAUDI+b41/PMsZOzI8/ayewfnSfQhBtaUmlj82aOtzjP9XtZZG4TPANw1avLHXFzbBIEAiwI=@vger.kernel.org X-Gm-Message-State: AOJu0YxwCKIskV+TRbeUEpy7ljDzgdAt2P+wBrHzeomZcvgbGqBK5Hmh avNkgb+ldjzf0suJwd4ngfMZdEGjn3eZdQlGQoMCtMJTHgD9ieMCZfy/ X-Gm-Gg: AeBDievWucgKZtmR/wxZMm7U+z5D12fVN5jBSVWPO2lw3v/QH6vpqnUOCLTkuJQSXEo 90ZjZUFpbo2j9vA1hU2wRYsLCzfL0J2iKfIVzoTjMiPZfiQ/1it+0pgOcVjnZMBcjUF+ZAj0bzC qkY/S9pelP8KR2pNPtgHTUJnvMLJdB48OGa5ahN/d1R7VoTaoij72cQKuUn3fqfDRtSc+6fcpzd cBdydtAvYuW3BMotkmvgiZHJIa51ijWRPWlJtbMF366IZe8rdyTTuq02a6USKb80A5dx8fmVCQK 9V5noK+1jx11fEqaA5xJEQ86B/b1hCAeCmiVhwJhA+Le79a3F+53cdFuiJ2EmUCxCdOWiagZOND CY54Bo2gmxcuPcG8aSbMcehen0r0H1bPQDmBLySTHVT6p8AkBqfhX+znPzZE3maAEnIceOmLayg EIXY0pFsRf7mi4Qa2W0I5kkJ7XPpoXMlMTqBqkIiZ71IIZ1AXg8M6kZpmTfPGfXXEdYzg83qg= X-Received: by 2002:a05:6820:1888:b0:68e:3f3b:8690 with SMTP id 006d021491bc7-696979feb08mr3582426eaf.21.1777841780608; Sun, 03 May 2026 13:56:20 -0700 (PDT) Received: from localhost (23-88-128-2.fttp.usinternet.com. [23.88.128.2]) by smtp.gmail.com with ESMTPSA id 006d021491bc7-69689440f5asm5146886eaf.1.2026.05.03.13.56.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 03 May 2026 13:56:20 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Sun, 03 May 2026 15:56:19 -0500 Message-Id: Subject: Re: [PATCH] tools: gpio: fix buffer overflow and add bounds check From: "Maxwell Doose" To: "Zhang Xiaolei" , Cc: , , X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20260503190016.13439-1-zxl434815272@gmail.com> In-Reply-To: <20260503190016.13439-1-zxl434815272@gmail.com> 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, u= nsigned int *lines, > int i; > int ret; > =20 > + if (!device_name || !lines || !config || !consumer || > + num_lines =3D=3D 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 =3D asprintf(&chrdev_name, "/dev/%s", device_name); > if (ret < 0) > return -ENOMEM; > =20 > - fd =3D open(chrdev_name, 0); > + fd =3D open(chrdev_name, O_RDONLY); > Another thing that can be split. > > if (fd =3D=3D -1) { > ret =3D -errno; > fprintf(stderr, "Failed to open %s, %s\n", > @@ -78,27 +82,29 @@ int gpiotools_request_line(const char *device_name, u= nsigned int *lines, > } > =20 > memset(&req, 0, sizeof(req)); > + > This seems like a stray change. > > for (i =3D 0; i < num_lines; i++) > req.offsets[i] =3D lines[i]; > =20 > req.config =3D *config; > - strcpy(req.consumer, consumer); > + strncpy(req.consumer, consumer, sizeof(req.consumer) - 1); > + req.consumer[sizeof(req.consumer) - 1] =3D '\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 =3D num_lines; > =20 > ret =3D ioctl(fd, GPIO_V2_GET_LINE_IOCTL, &req); > if (ret =3D=3D -1) { > ret =3D -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. > > } > =20 > if (close(fd) =3D=3D -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