Received-SPF: pass (google.com: domain of linux-kernel+bounces-188693-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 24 May 2024 14:38:04 +0200
From: Jiri Bohac <jbohac@suse.cz>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: cve@kernel.org, linux-kernel@vger.kernel.org,
	linux-cve-announce@vger.kernel.org,
	Eric Biederman <ebiederm@xmission.com>, kexec@lists.infradead.org
Subject: Re: CVE-2023-52823: kernel: kexec: copy user-array safely
Message-ID: <ZlCKLBjrF5PWt1hz@dwarf.suse.cz>
References: <2024052106-CVE-2023-52823-3d81@gregkh>
 <ZlBlorsBMPK0RdnR@dwarf.suse.cz>
 <2024052420-clang-flatterer-366b@gregkh>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2024052420-clang-flatterer-366b@gregkh>

On Fri, May 24, 2024 at 12:15:47PM +0200, Greg Kroah-Hartman wrote:
> Nice, but then why was this commit worded this way?  Now we check twice?
> Double safe?  Should it be reverted?

double safe's good; turning it into a CVE not so much :(
CVE-2023-52822, CVE-2023-52824 and CVE-2023-52820, originally from the same patch
series, seem to be the exact same case.

CVE-2023-52822:

	int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
				     struct drm_file *file_priv)
	{
	...
		if (num_sizes > DRM_VMW_MAX_SURFACE_FACES * DRM_VMW_MAX_MIP_LEVELS ||
		    num_sizes == 0)
			return -EINVAL;
	...
		metadata->num_sizes = num_sizes;
		metadata->sizes =
			memdup_user((struct drm_vmw_size __user *)(unsigned long)
				    req->size_addr,
				    sizeof(*metadata->sizes) * metadata->num_sizes);
	}

CVE-2023-52824 (here the check is in the immediately preceeding statement, could it
be any more obvious?):

	long watch_queue_set_filter(struct pipe_inode_info *pipe,
				    struct watch_notification_filter __user *_filter)
	{
		if (filter.nr_filters == 0 ||
		    filter.nr_filters > 16 ||
		    filter.__reserved != 0)
			return -EINVAL;

		tf = memdup_user(_filter->filters, filter.nr_filters * sizeof(*tf));
	}


CVE-2023-52820 is a little less obvious to be safe, but I believe it is:

	int drm_mode_create_lease_ioctl(struct drm_device *dev,
					void *data, struct drm_file *lessor_priv)
	{
	...
			object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
						 array_size(object_count, sizeof(__u32)));

	array_size() will safely multiply object_count * 4 and return SIZE_MAX on
	overflow, making the kmalloc inside memdup_user cleanly fail with -ENOMEM.


> I'll go revoke this, thanks for the review!

could you check and revoke all the above as well?

Thanks,

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, Prague, Czechia