Re: CVE-2023-52823: kernel: kexec: copy user-array safely

Re: CVE-2023-52823: kernel: kexec: copy user-array safely

[Jiri Bohac]

On Tue, May 21, 2024 at 05:31:59PM +0200, Greg Kroah-Hartman wrote:
> kernel: kexec: copy user-array safely
> 
> Currently, there is no overflow-check with memdup_user().

This is false. 
Therefore, I'd like to dispute this CVE.

The overflow check is in the kexec_load_check()
function called shortly before the memdup_user() call:


	SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
			struct kexec_segment __user *, segments, unsigned long, flags)
	{
		result = kexec_load_check(nr_segments, flags);
		if (result)
			return result;
	...
		ksegments = memdup_user(segments, nr_segments * sizeof(ksegments[0]));
	...
	}

	#define KEXEC_SEGMENT_MAX 16
	static inline int kexec_load_check(unsigned long nr_segments,
					   unsigned long flags)
	{
	...
		if (nr_segments > KEXEC_SEGMENT_MAX)
			return -EINVAL;
	}



Thanks,

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: CVE-2023-52823: kernel: kexec: copy user-array safely

[Greg Kroah-Hartman]

On Fri, May 24, 2024 at 12:02:10PM +0200, Jiri Bohac wrote:
> On Tue, May 21, 2024 at 05:31:59PM +0200, Greg Kroah-Hartman wrote:
> > kernel: kexec: copy user-array safely
> > 
> > Currently, there is no overflow-check with memdup_user().
> 
> This is false. 
> Therefore, I'd like to dispute this CVE.
> 
> The overflow check is in the kexec_load_check()
> function called shortly before the memdup_user() call:
> 
> 
> 	SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
> 			struct kexec_segment __user *, segments, unsigned long, flags)
> 	{
> 		result = kexec_load_check(nr_segments, flags);
> 		if (result)
> 			return result;
> 	...
> 		ksegments = memdup_user(segments, nr_segments * sizeof(ksegments[0]));
> 	...
> 	}
> 
> 	#define KEXEC_SEGMENT_MAX 16
> 	static inline int kexec_load_check(unsigned long nr_segments,
> 					   unsigned long flags)
> 	{
> 	...
> 		if (nr_segments > KEXEC_SEGMENT_MAX)
> 			return -EINVAL;
> 	}

Nice, but then why was this commit worded this way?  Now we check twice?
Double safe?  Should it be reverted?

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

greg k-h

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: CVE-2023-52823: kernel: kexec: copy user-array safely

[Jiri Bohac]

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: CVE-2023-52823: kernel: kexec: copy user-array safely

[Jiri Bohac]

On Fri, May 24, 2024 at 02:38:04PM +0200, Jiri Bohac wrote:
> 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.

Same thing: CVE-2023-52758


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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: CVE-2023-52823: kernel: kexec: copy user-array safely

[Greg Kroah-Hartman]

On Fri, May 24, 2024 at 02:38:04PM +0200, Jiri Bohac wrote:
> 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);
> 	}

Agreed, now rejected.

> 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));
> 	}

Yup, now rejected.

> 
> 
> 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.

Also agreed, now rejected.

thanks,

greg k-h

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: CVE-2023-52823: kernel: kexec: copy user-array safely

[Greg Kroah-Hartman]

On Fri, May 24, 2024 at 04:13:53PM +0200, Jiri Bohac wrote:
> On Fri, May 24, 2024 at 02:38:04PM +0200, Jiri Bohac wrote:
> > 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.
> 
> Same thing: CVE-2023-52758

Agreed, now rejected, thanks for the review!

greg k-h

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec