Re: [PATCH hurd] pci-arbiter: Fix long standing bug with PCI access

[Sergey Bugaev]

On Sat, Dec 28, 2024 at 9:38 AM Damien Zammit via Bug reports for the
GNU Hurd <bug-hurd@gnu.org> wrote:
>
> Proxied memory was not rounded up to page size, causing
> error with vm_map'ing  the underlying memory.
>
> WARNING: Could be security risk if assumption is incorrect:
> Assumes start of all pci memory resources are at least page aligned.
>
> ---
>  pci-arbiter/netfs_impl.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
> index 4bb5c97a..50e527cf 100644
> --- a/pci-arbiter/netfs_impl.c
> +++ b/pci-arbiter/netfs_impl.c
> @@ -577,6 +577,7 @@ get_filemap_region (struct node *node, vm_prot_t prot)
>    vm_prot_t max_prot;
>    size_t reg_num;
>    struct pci_mem_region *region;
> +  size_t rest = sysconf (_SC_PAGESIZE);

Should be the same thing, but I'd rather use vm_page_size, we're
operating on Mach level here, and sysconf is too Unixy.

>    /* Get region info */
>    reg_num =
> @@ -592,12 +593,20 @@ get_filemap_region (struct node *node, vm_prot_t prot)
>    if (err)
>      goto error;
>
> +  /* WARNING: this rounds up the proxied region to a whole page.
> +   * This may be a security risk, but is the only way to provide access
> +   * to all of the memory region.  (We assume pci memory is at least page 
> aligned). */
> +  if (region->size % rest)
> +    rest -= region->size % rest;
> +  else
> +    rest = 0;

Perhaps just

roundup (region->size, vm_page_size)

?

> +
>    /* Create a new memory object proxy with the required protection */
>    max_prot = (VM_PROT_READ | VM_PROT_WRITE) & prot;
>    err =
>      vm_region_create_proxy (mach_task_self (),
>                             (vm_address_t) node->nn->ln->region_maps[reg_num],
> -                           max_prot, region->size, &proxy);
> +                           max_prot, region->size + rest, &proxy);
>    if (err)
>      goto error;

Sergey