Re: [PATCH] staging: comedi: cast to (unsigned int *)

[Ian Abbott]

On 17/02/2021 18:26, Greg KH wrote:
> On Wed, Feb 17, 2021 at 11:40:00PM +0530, Atul Gopinathan wrote:
> > On Wed, Feb 17, 2021 at 06:35:15PM +0100, Greg KH wrote:
> > > On Wed, Feb 17, 2021 at 10:29:08PM +0530, Atul Gopinathan wrote:
> > > > Resolve the following warning generated by sparse:
> > > >
> > > > drivers/staging//comedi/comedi_fops.c:2956:23: warning: incorrect type in assignment (different address spaces)
> > > > drivers/staging//comedi/comedi_fops.c:2956:23:    expected unsigned int *chanlist
> > > > drivers/staging//comedi/comedi_fops.c:2956:23:    got void [noderef] <asn:1> *
> > > >
> > > > compat_ptr() has a return type of "void __user *"
> > > > as defined in "include/linux/compat.h"
> > > >
> > > > cmd->chanlist is of type "unsigned int *" as defined
> > > > in drivers/staging/comedi/comedi.h" in struct
> > > > comedi_cmd.
> > > >
> > > > Signed-off-by: Atul Gopinathan <atulgopinathan@xxxxxxxxx>
> > > > ---
> > > >   drivers/staging/comedi/comedi_fops.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> > > > index e85a99b68f31..fc4ec38012b4 100644
> > > > --- a/drivers/staging/comedi/comedi_fops.c
> > > > +++ b/drivers/staging/comedi/comedi_fops.c
> > > > @@ -2953,7 +2953,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd,
> > > >   	cmd->scan_end_arg = v32.scan_end_arg;
> > > >   	cmd->stop_src = v32.stop_src;
> > > >   	cmd->stop_arg = v32.stop_arg;
> > > > -	cmd->chanlist = compat_ptr(v32.chanlist);
> > > > +	cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist);
> > >
> > > __force?  That feels wrong, something is odd if that is ever needed.
> > >
> > > Are you _sure_ this is correct?
> >
> > The same file has instances of "(usigned int __force *)" cast being
> > used on the same "cmd->chanlist". For reference:
> >
> > At line 1797 of comedi_fops.c:
> > 1796                 /* restore chanlist pointer before copying back */
> > 1797                 cmd->chanlist = (unsigned int __force *)user_chanlist;
> > 1798                 cmd->data = NULL;
> >
> > At line 1880:
> > 1879         /* restore chanlist pointer before copying back */
> > 1880         cmd->chanlist = (unsigned int __force *)user_chanlist;
> > 1881         *copy = true;
> >
> > Here "user_chanlist" is of type "unsigned int __user *".
> >
> >
> > Or perhaps, I shouldn't be relying on them?
>
> I don't know, it still feels wrong.
>
> Ian, any thoughts?

It's kind of moot anyway because the patch is outdated.  But the reason 
for the ___force is that the same `struct comedi_cmd` is used in both 
user and kernel contexts.  In user contexts, the `chanlist` member 
points to user memory and in kernel contexts it points to kernel memory 
(copied from userspace).

The sparse tagging of this member has flip-flopped a bit over the years:

* commit 92d0127c9d24 ("Staging: comedi: __user markup on 
comedi_fops.c") (May 2010) tagged it as `__user`.

* commit 9be56c643263 ("staging: comedi: comedi.h: remove __user tag 
from chanlist") (Sep 2012) removed the `__user` tag.

It is mostly used in a kernel context, for example all the low-level 
drivers with `do_cmd` and `do_cmdtest` handlers use it in kernel context.

The alternative would be to have a separate kernel version of this 
struct, but it would be mostly identical to the user version apart from 
the sparse tagging of this member and perhaps the removal of the unused 
`data` and `data_len` members (which need to be kept in the user version 
of the struct for compatibility reasons).

--
-=( Ian Abbott <abbotti@xxxxxxxxx> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-