Received-SPF: pass (google.com: domain of linux-kernel+bounces-183016-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <03cc262da2a3db817aa5663fbce6c914708b74f8.camel@perches.com>
Subject: Re: [PATCH] drm/nouveau/nvif: Avoid build error due to potential
 integer overflows
From: Joe Perches <joe@perches.com>
To: Guenter Roeck <linux@roeck-us.net>, Kees Cook <keescook@chromium.org>, 
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: airlied@gmail.com, dakr@redhat.com, daniel@ffwll.ch, 
 dri-devel@lists.freedesktop.org, jani.nikula@intel.com, javierm@redhat.com,
  kherbst@redhat.com, linux-kernel@vger.kernel.org, lyude@redhat.com, 
 mripard@kernel.org, nouveau@lists.freedesktop.org, tzimmermann@suse.de, 
 linux-hardening@vger.kernel.org
Date: Sat, 18 May 2024 18:19:18 -0700
In-Reply-To: <a912c2d1-9008-410a-92cc-912e17c63695@roeck-us.net>
References: <20240518143743.313872-1-linux@roeck-us.net>
	 <34a6d812-b4ed-4465-b0ec-e641d185b9b1@wanadoo.fr>
	 <202405181020.2D0A364F@keescook>
	 <a912c2d1-9008-410a-92cc-912e17c63695@roeck-us.net>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
Precedence: bulk
MIME-Version: 1.0

On Sat, 2024-05-18 at 11:23 -0700, Guenter Roeck wrote:
> On 5/18/24 10:32, Kees Cook wrote:
>=20
[]
> > I think the INT_MAX test is actually better in this case because
> > nvif_object_ioctl()'s size argument is u32:
> >=20
> > ret =3D nvif_object_ioctl(object, args, sizeof(*args) + size, NULL);
> >                                        ^^^^^^^^^^^^^^^^^^^^
> >=20
> > So that could wrap around, even though the allocation may not.
> >=20
> > Better yet, since "sizeof(*args) + size" is repeated 3 times in the
> > function, I'd recommend:
> >=20
> > 	...
> > 	u32 args_size;
> >=20
> > 	if (check_add_overflow(sizeof(*args), size, &args_size))
> > 		return -ENOMEM;
> > 	if (args_size > sizeof(stack)) {
> > 		if (!(args =3D kmalloc(args_size, GFP_KERNEL)))

trivia:

More typical kernel style would use separate alloc and test

		args =3D kmalloc(args_size, GFP_KERNEL);
		if (!args)

> > 			return -ENOMEM;
> >          } else {
> >                  args =3D (void *)stack;
> >          }
> > 	...
> >          ret =3D nvif_object_ioctl(object, args, args_size, NULL);
> >=20
> > This will catch the u32 overflow to nvif_object_ioctl(), catch an
> > allocation underflow on 32-bits systems, and make the code more
> > readable. :)
> >=20
>=20
> Makes sense. I'll change that and send v2.
>=20
> Thanks,
> Guenter
>=20
>=20