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 <[email protected]>
SUSE Labs, Prague, Czechia
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
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 <[email protected]>
SUSE Labs, Prague, Czechia
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 <[email protected]>
SUSE Labs, Prague, Czechia
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
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