[PATCH] char: ppdev: fixed a validation issue

[PATCH] char: ppdev: fixed a validation issue

[Vihas Mak]

Make sure the mode is a valid IEEE1284 mode.

Signed-off-by: Vihas Mak <makvihas@gmail.com>
---
 drivers/char/ppdev.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index 38b46c7d1737..3b290cbf6c66 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -333,6 +333,28 @@ static enum ieee1284_phase init_phase(int mode)
 	return IEEE1284_PH_FWD_IDLE;
 }
 
+/*
+ * Validate the mode and make sure the mode is power of two.
+ *
+ * IEEE1284_MODE_ECPRLE and IEEE1284_MODE_NIBBLE are exception
+ * to this so handle them accordingly.
+ */
+
+static int pp_validate_mode(int mode)
+{
+	if (mode == IEEE1284_MODE_ECPRLE || mode == IEEE1284_MODE_NIBBLE) {
+		return 1;
+	} else if (!(mode & (mode - 1)) &&
+		   (mode & (IEEE1284_MODE_BYTE | IEEE1284_MODE_COMPAT |
+			    IEEE1284_MODE_BECP | IEEE1284_MODE_ECP |
+			    IEEE1284_MODE_ECPSWE | IEEE1284_MODE_EPP |
+			    IEEE1284_MODE_EPPSL | IEEE1284_MODE_COMPAT |
+			    IEEE1284_MODE_EPPSWE))) {
+		return 1;
+	}
+	return 0;
+}
+
 static int pp_set_timeout(struct pardevice *pdev, long tv_sec, int tv_usec)
 {
 	long to_jiffies;
@@ -423,7 +445,11 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		if (copy_from_user(&mode, argp, sizeof(mode)))
 			return -EFAULT;
-		/* FIXME: validate mode */
+
+		/* Validate mode */
+		if (!pp_validate_mode(mode))
+			return -EINVAL;
+
 		pp->state.mode = mode;
 		pp->state.phase = init_phase(mode);
 
-- 
2.25.1

Re: [PATCH] char: ppdev: fixed a validation issue

[Greg KH]

On Tue, Nov 09, 2021 at 12:28:18AM +0530, Vihas Mak wrote:
> Make sure the mode is a valid IEEE1284 mode.

What is a valid mode?

> Signed-off-by: Vihas Mak <makvihas@gmail.com>
> ---
>  drivers/char/ppdev.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index 38b46c7d1737..3b290cbf6c66 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -333,6 +333,28 @@ static enum ieee1284_phase init_phase(int mode)
>  	return IEEE1284_PH_FWD_IDLE;
>  }
>  
> +/*
> + * Validate the mode and make sure the mode is power of two.
> + *
> + * IEEE1284_MODE_ECPRLE and IEEE1284_MODE_NIBBLE are exception
> + * to this so handle them accordingly.
> + */
> +

Why the extra line?

> +static int pp_validate_mode(int mode)

bool?

> +{
> +	if (mode == IEEE1284_MODE_ECPRLE || mode == IEEE1284_MODE_NIBBLE) {
> +		return 1;
> +	} else if (!(mode & (mode - 1)) &&
> +		   (mode & (IEEE1284_MODE_BYTE | IEEE1284_MODE_COMPAT |
> +			    IEEE1284_MODE_BECP | IEEE1284_MODE_ECP |
> +			    IEEE1284_MODE_ECPSWE | IEEE1284_MODE_EPP |
> +			    IEEE1284_MODE_EPPSL | IEEE1284_MODE_COMPAT |
> +			    IEEE1284_MODE_EPPSWE))) {
> +		return 1;
> +	}
> +	return 0;
> +}

How did you test this?  And why is this needed now?  What hardware was
working that is now not going to work with this driver?

thanks,

greg k-h

Re: [PATCH] char: ppdev: fixed a validation issue

[Vihas Mak]

> On Tue, Nov 09, 2021 at 12:28:18AM +0530, Vihas Mak wrote:
> > Make sure the mode is a valid IEEE1284 mode.
> What is a valid mode?

The valid IEEE1284 modes are the ones defined in
<uapi/linux/parport.h>. Currently there are 10 modes. Namely nibble
mode, byte mode, ECP, ECPRLE, EPP and some specials.

> How did you test this?  And why is this needed now?  What hardware was
> working that is now not going to work with this driver?

I tested this on my local pc using a parallel port and read the
incoming data on my Raspberry PI.
I also used https://github.com/strezh/VPPSniffer. It's a simple
virtual parallel port used for debugging and sniffing.

The mainline code wasn't validating the mode when a user-space program
does a ioctl call to change the current mode. It might
create some bugs if the new mode isn't one of the IEEE1284 modes
defined in <uapi/linux/parport.h>. So it's better to throw a EINVAL
beforehand, if the mode is invalid.

> > +static int pp_validate_mode(int mode)
> bool?

My bad. Will do a v2.

Thanks,
Vihas

On Tue, Nov 9, 2021 at 11:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Nov 09, 2021 at 12:28:18AM +0530, Vihas Mak wrote:
> > Make sure the mode is a valid IEEE1284 mode.
>
> What is a valid mode?
>
> > Signed-off-by: Vihas Mak <makvihas@gmail.com>
> > ---
> >  drivers/char/ppdev.c | 28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> > index 38b46c7d1737..3b290cbf6c66 100644
> > --- a/drivers/char/ppdev.c
> > +++ b/drivers/char/ppdev.c
> > @@ -333,6 +333,28 @@ static enum ieee1284_phase init_phase(int mode)
> >       return IEEE1284_PH_FWD_IDLE;
> >  }
> >
> > +/*
> > + * Validate the mode and make sure the mode is power of two.
> > + *
> > + * IEEE1284_MODE_ECPRLE and IEEE1284_MODE_NIBBLE are exception
> > + * to this so handle them accordingly.
> > + */
> > +
>
> Why the extra line?
>
> > +static int pp_validate_mode(int mode)
>
> bool?
>
> > +{
> > +     if (mode == IEEE1284_MODE_ECPRLE || mode == IEEE1284_MODE_NIBBLE) {
> > +             return 1;
> > +     } else if (!(mode & (mode - 1)) &&
> > +                (mode & (IEEE1284_MODE_BYTE | IEEE1284_MODE_COMPAT |
> > +                         IEEE1284_MODE_BECP | IEEE1284_MODE_ECP |
> > +                         IEEE1284_MODE_ECPSWE | IEEE1284_MODE_EPP |
> > +                         IEEE1284_MODE_EPPSL | IEEE1284_MODE_COMPAT |
> > +                         IEEE1284_MODE_EPPSWE))) {
> > +             return 1;
> > +     }
> > +     return 0;
> > +}
>
> How did you test this?  And why is this needed now?  What hardware was
> working that is now not going to work with this driver?
>
> thanks,
>
> greg k-h

Re: [PATCH] char: ppdev: fixed a validation issue

[Greg KH]

On Tue, Nov 09, 2021 at 04:11:20PM +0530, Vihas Mak wrote:
> > On Tue, Nov 09, 2021 at 12:28:18AM +0530, Vihas Mak wrote:
> > > Make sure the mode is a valid IEEE1284 mode.
> > What is a valid mode?
> 
> The valid IEEE1284 modes are the ones defined in
> <uapi/linux/parport.h>. Currently there are 10 modes. Namely nibble
> mode, byte mode, ECP, ECPRLE, EPP and some specials.

But what happens today if code has not been sending those values
properly?  This is a very old driver for very old hardware.

> > How did you test this?  And why is this needed now?  What hardware was
> > working that is now not going to work with this driver?
> 
> I tested this on my local pc using a parallel port and read the
> incoming data on my Raspberry PI.
> I also used https://github.com/strezh/VPPSniffer. It's a simple
> virtual parallel port used for debugging and sniffing.
> 
> The mainline code wasn't validating the mode when a user-space program
> does a ioctl call to change the current mode. It might
> create some bugs if the new mode isn't one of the IEEE1284 modes
> defined in <uapi/linux/parport.h>. So it's better to throw a EINVAL
> beforehand, if the mode is invalid.

What happens today if the mode is not set properly?  Will the code paths
error out eventually, or will the call succeed?  The problem is that
there might be code that is working today that would break with a change
like this, as again, this is a very old driver.

thanks,

greg k-h

Re: [PATCH] char: ppdev: fixed a validation issue

[Vihas Mak]

> What happens today if the mode is not set properly?  Will the code paths
> error out eventually, or will the call succeed?  The problem is that
> there might be code that is working today that would break with a change
> like this, as again, this is a very old driver.

I see. So I guess this driver might be better off without any changes,
as new changes might break things more severely.

Thanks,
Vihas

On Tue, Nov 9, 2021 at 4:53 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Nov 09, 2021 at 04:11:20PM +0530, Vihas Mak wrote:
> > > On Tue, Nov 09, 2021 at 12:28:18AM +0530, Vihas Mak wrote:
> > > > Make sure the mode is a valid IEEE1284 mode.
> > > What is a valid mode?
> >
> > The valid IEEE1284 modes are the ones defined in
> > <uapi/linux/parport.h>. Currently there are 10 modes. Namely nibble
> > mode, byte mode, ECP, ECPRLE, EPP and some specials.
>
> But what happens today if code has not been sending those values
> properly?  This is a very old driver for very old hardware.
>
> > > How did you test this?  And why is this needed now?  What hardware was
> > > working that is now not going to work with this driver?
> >
> > I tested this on my local pc using a parallel port and read the
> > incoming data on my Raspberry PI.
> > I also used https://github.com/strezh/VPPSniffer. It's a simple
> > virtual parallel port used for debugging and sniffing.
> >
> > The mainline code wasn't validating the mode when a user-space program
> > does a ioctl call to change the current mode. It might
> > create some bugs if the new mode isn't one of the IEEE1284 modes
> > defined in <uapi/linux/parport.h>. So it's better to throw a EINVAL
> > beforehand, if the mode is invalid.
>
> What happens today if the mode is not set properly?  Will the code paths
> error out eventually, or will the call succeed?  The problem is that
> there might be code that is working today that would break with a change
> like this, as again, this is a very old driver.
>
> thanks,
>
> greg k-h

Re: [PATCH] char: ppdev: fixed a validation issue

[Greg KH]

On Tue, Nov 09, 2021 at 09:04:57PM +0530, Vihas Mak wrote:
> > What happens today if the mode is not set properly?  Will the code paths
> > error out eventually, or will the call succeed?  The problem is that
> > there might be code that is working today that would break with a change
> > like this, as again, this is a very old driver.
> 
> I see. So I guess this driver might be better off without any changes,
> as new changes might break things more severely.

Changes in behavior always have the ability to break things.  What other
changes do you want to make to this old driver that might cause
problems?

And you didn't answer my question above, what happens if the mode is not
set properly today?

thanks,

greg k-h

Re: [PATCH] char: ppdev: fixed a validation issue

[Vihas Mak]

> And you didn't answer my question above, what happens if the mode is not
> set properly today?

It'll throw a ENOSYS if the mode is not set properly and print the following:

pr_debug("%s: Unknown mode 0x%02x\n",
             port->name, port->ieee1284.mode);


On Tue, Nov 9, 2021 at 9:11 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Nov 09, 2021 at 09:04:57PM +0530, Vihas Mak wrote:
> > > What happens today if the mode is not set properly?  Will the code paths
> > > error out eventually, or will the call succeed?  The problem is that
> > > there might be code that is working today that would break with a change
> > > like this, as again, this is a very old driver.
> >
> > I see. So I guess this driver might be better off without any changes,
> > as new changes might break things more severely.
>
> Changes in behavior always have the ability to break things.  What other
> changes do you want to make to this old driver that might cause
> problems?
>
> And you didn't answer my question above, what happens if the mode is not
> set properly today?
>
> thanks,
>
> greg k-h

Re: [PATCH] char: ppdev: fixed a validation issue

[Greg KH]

On Tue, Nov 09, 2021 at 11:21:39PM +0530, Vihas Mak wrote:
> > And you didn't answer my question above, what happens if the mode is not
> > set properly today?
> 
> It'll throw a ENOSYS if the mode is not set properly and print the following:
> 
> pr_debug("%s: Unknown mode 0x%02x\n",
>              port->name, port->ieee1284.mode);

Ah, good, so then your change is not needed at all, so why add it?

thanks,

greg k-h