linux-kernel - Re: [RFC PATCH 1/1] s390/cio: make ccw_device_dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Oct 2021 20:42:49 +0200
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Pierre Morel <pmorel@...ux.ibm.com>
Cc:     Vineeth Vijayan <vneethv@...ux.ibm.com>,
        Peter Oberparleiter <oberpar@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Michael Mueller <mimu@...ux.ibm.com>,
        Cornelia Huck <cohuck@...hat.com>, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        bfu@...hat.com, Halil Pasic <pasic@...ux.ibm.com>
Subject: Re: [RFC PATCH 1/1] s390/cio: make ccw_device_dma_* more robust

On Mon, 11 Oct 2021 15:45:55 +0200
Pierre Morel <pmorel@...ux.ibm.com> wrote:

> > +	if (IS_ERR_OR_NULL(addr))  
> 
> I can be wrong but it seems that only dma_alloc_coherent() used in 
> cio_gp_dma_zalloc() report an error but the error is ignored and used as 
> a valid pointer.


https://www.kernel.org/doc/Documentation/DMA-API.txt says:

Part Ia - Using large DMA-coherent buffers
------------------------------------------

::

	void *
	dma_alloc_coherent(struct device *dev, size_t size,
			   dma_addr_t *dma_handle, gfp_t flag)

[..]

It returns a pointer to the allocated region (in the processor's virtual
address space) or NULL if the allocation failed.

I hope that is still true. If not we should fix cio_gp_dma_zalloc().

> 
> So shouldn't we modify this function and just test for a NULL address here?
> 

Isn't IS_ERR_OR_NULL() safer, in a sense that even if we decided to
eventually return an error code, this piece of code would be robust
and safe?

We may exploit the knowledge that cio_gp_dma_zalloc() either
returns NULL or a valid pointer, but doing it like this is IMHO also an
option.

> here what I mean:---------------------------------
> 
> diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> index 2bc55ccf3f23..b45fbaa7131b 100644
> --- a/drivers/s390/cio/css.c
> +++ b/drivers/s390/cio/css.c
> @@ -1176,7 +1176,7 @@ void *cio_gp_dma_zalloc(struct gen_pool *gp_dma, 
> struct device *dma_dev,
>                  chunk_size = round_up(size, PAGE_SIZE);
>                  addr = (unsigned long) dma_alloc_coherent(dma_dev,
>                                           chunk_size, &dma_addr, 
> CIO_DMA_GFP);
> -               if (!addr)
> +               if (IS_ERR_OR_NULL(addr))
>                          return NULL;
>                  gen_pool_add_virt(gp_dma, addr, dma_addr, chunk_size, -1);
>                  addr = gen_pool_alloc(gp_dma, size);
> 
> ---------------------------------
> 
> > +		put_device(&cdev->dev);  
> 
> addr is not null if addr is ERR.
> 

Your point?

> > +	return addr;  
> 
> may be return IS_ERR_OR_NULL(addr)? NULL : addr;
> 

See above. I don't think that is necessary.

> >   }
> >   EXPORT_SYMBOL(ccw_device_dma_zalloc);
> >   
> >   void ccw_device_dma_free(struct ccw_device *cdev, void *cpu_addr, size_t size)
> >   {
> > +	if (!cpu_addr)
> > +		return;  
> 
> no need, cpu_addr is already tested in cio_gp_dma_free()
> 

This is added in because of the put_device(). An alternative would be
to call cio_gp_dma_free() unconditionally do the check just for the
put_device(). But I like this one better.

Thanks for your feedback!

Halil

> >   	cio_gp_dma_free(cdev->private->dma_pool, cpu_addr, size);
> > +	put_device(&cdev->dev);
> >   }
> >   EXPORT_SYMBOL(ccw_device_dma_free);
> >   
> >