Re: [RFC PATCH] x86/sev: Disallow userspace access to BIOS region for SEV-SNP guests

[Dan Williams <dan.j.williams@intel.com>]

Nikolay Borisov wrote:
> 
> 
> On 10.04.25 г. 22:20 ч., Dan Williams wrote:
> > Nikolay Borisov wrote:
> > [..]
> >>>> Can't we simply treat return value of 2 for range_is_allowed the same way as
> >>>> if 0 was returned in mmap_mem and simply fail the call with -EPERM?
> >>>
> >>> The historical concern was that EPERM would break old tools. I don't
> >>> have any current evidence either way, though.
> >>
> >> Right, but we are only about to return 2 in a TVM context, so chances of
> >> running old tools are slim to none. Also it's perfectly valid to have
> >> mmap fail for a number of reasons, so old tools should be equipped with
> >> handling it returning -EPERM, no ?
> > 
> > In practice that is yet another return code since the caller does not
> > know why the "2" is being returned and it is not clear how safe it is to
> > now start denying mmap in the !TVM case. So, perhaps something like this:
> > 
> 
> What I meant by "returning 2" is returning 2 from the call to 
> range_is_allowed in mmap_mem and handling this value inside mmap_mem, 
> not returning 2 to user space :)

Oh, no, I was not confused about that, just the conflict that "2" means
that mmap is ok currently.

> In essence something along the lines of:
> 
> 
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 169eed162a7f..8273066b6637 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -359,7 +359,8 @@ static int mmap_mem(struct file *file, struct 
> vm_area_struct *vma)
>          if (!private_mapping_ok(vma))
>                  return -ENOSYS;
> 
> -       if (!range_is_allowed(vma->vm_pgoff, size))
> +       int ret = range_is_allowed(vma->vm_pgoff, size);
> +       if (!ret || ret == 2)
>                  return -EPERM;

Right, the issue is that this potentially breaks userspace that had
glommed onto the idea that it can always mmap BIOS data even if the R/W
path returns zeros.

It is arguably a bug that we allow that bypass, but it has been shipping
for a while. I think it is reasonable for TVMs to try to shut this down
completely, but the question is whether doing this instead:

   if (!ret || ret == 3)

...allows the TVM case to not disturb legacy expectations?

However, my vote is to special case 2 as EPERM as you have it here.
Because, if that works, it solves both the TVM need and silently closes
this weird hole hopefully before userspace actually starts depending on
it. We can always switch to 3 or do the work to map zeros if that proves
to be a regression.