[PATCH] KVM: arm64: Fix buffer overflow in kvm_arm_set_fw_reg()

[Dan Carpenter]

On Wed, Apr 19, 2023 at 11:00:37AM +0100, Steven Price wrote:
> On 19/04/2023 10:48, Dan Carpenter wrote:
> > On Wed, Apr 19, 2023 at 09:48:41AM +0100, Steven Price wrote:
> >> On 19/04/2023 09:06, Dan Carpenter wrote:
> >>> The KVM_REG_SIZE() comes from the ioctl and it can be a power of two
> >>> between 0-32768 but if it is more than sizeof(long) this will corrupt
> >>> memory.
> >>>
> >>> Fixes: 99adb567632b ("KVM: arm/arm64: Add save/restore support for firmware workaround state")
> >>> Signed-off-by: Dan Carpenter <dan.carpenter at linaro.org>
> >>
> >> Reviewed-by: Steven Price <steven.price at arm.com>
> >>
> >> Although there might be something to be said for rejecting anything
> >> where KVM_REG_SIZE(reg->id) != sizeof(u64), as for smaller sizes the top
> >> bits of val would be undefined which would require the code to mask the
> >> top bits out to be safe. Given that all registers are currently u64 (and
> >> I don't expect that to change), perhaps the below would be clearer?
> >>
> >> 	if (KVM_REG_SIZE(reg->id) != sizeof(val))
> >> 		return -EINVAL;
> >> 	if (copy_from_user(&val, uaddr, sizeof(val)))
> >> 		return -EFAULT;
> > 
> > I was thinking that zero might be a valid size?
> 
> Well any value of reg->id which doesn't match in the switch statement
> will cause a -ENOENT return currently, so a zero size would fail that
> way as it stands. So I don't think any size other than u64 is valid in
> the current code.
> 
> There is obviously a question of return value - perhaps returning
> -ENOENT would be more appropriate if the size doesn't match (as a later
> kernel could decide to implement registers of different sizes)?

Okay.  I've sent a v2.  Probably either -EINVAL or -ENOENT is fine, but
-ENOENT is more helpful so let's return that.

regards,
dan carpenter