RE: Serial.c BUG 2.4.x-2.5x

RE: Serial.c BUG 2.4.x-2.5x

[Ed Vance]

On Thu Mar 07, 2002, Roman Kurakin wrote:
> 
> On Wed Mar 06, 2002, Russell King wrote:
> >
> >The patch does fine for the most part, but I have two worries:
> >
> >1. the possibilities of pushing through changes in the IO or memory space
> >   by changing the other space at the same time. (ie, port = 1, iomem =
> >   0xfe007c00 and you already have a line at port = 0, iomem =
0xfe007c00).
> >   I dealt with this properly using the resource management subsystem.
> >
> I think such code could solve this problem ...
> 
> - 	    (rs_table[i].port == new_port) &&
> + 	    ((rs_table[i].port && rs_table[i].port == new_port) ||
> +	    ((rs_table[i].iomem_base && rs_table[i].iomem_base == new_mem))
&&

Indeed it would solve this problem, but I'm not sure there is a problem to
solve here. Have not found a case where ->port and ->iomem_base fields can
both be non-zero. If one of them is always zero then the previous patch hunk
in the "address in use" test at about line 2146 is well enough:

            if ((state != &rs_table[i]) &&
                (rs_table[i].port == new_port) &&
+               (rs_table[i].iomem_base == new_mem) &&
                rs_table[i].type)
                    return -EADDRINUSE;

Assuming one of the two fields is always zero, demanding both to match for
the in use condition works anyway. If the non-zero field matches, then they
both must match. The following hunk at the bottom of function get_pci_port()
at about line 3931 seems to guarantee that they start out this way:

	[[ The req struct is memset() to zero at about line 4009 in 
	  function start_pci_pnp_board(). ]]

        if (IS_PCI_REGION_IOPORT(dev, base_idx)) {
                req->port = port;
                if (HIGH_BITS_OFFSET)
                        req->port_high = port >> HIGH_BITS_OFFSET;
                else
                        req->port_high = 0;
                return 0;
        }
        req->io_type = SERIAL_IO_MEM;
        req->iomem_base = ioremap(port, board->uart_offset);
        req->iomem_reg_shift = board->reg_shift;
        req->port = 0;
        return 0;

Does anybody see a need to add the code anyway? Did I miss a lurker? 

Best to all,

---------------------------------------------------------------- 
Ed Vance              serial24@macrolink.com
Macrolink, Inc.       1500 N. Kellogg Dr  Anaheim, CA  92807
----------------------------------------------------------------

RE: Serial.c BUG 2.4.x-2.5x

[Ed Vance]

Thanks Russell and Roman for your helpful input. I'll look at it today and
resubmit for further discussion.

Ed Vance

Roman Kurakin wrote:

> Russell King wrote:
> 
> >The patch does fine for the most part, but I have two worries:
> >
> >1. the possibilities of pushing through changes in the IO or memory space
> >   by changing the other space at the same time. (ie, port = 1, iomem =
> >   0xfe007c00 and you already have a line at port = 0, iomem =
0xfe007c00).
> >   I delt with this properly using the resource management subsystem.
> >
> I think such code could solve this problem ...
> 
> - 			    (rs_table[i].port == new_port) &&
> + 			    ((rs_table[i].port && rs_table[i].port ==
new_port) ||
> +			    ((rs_table[i].iomem_base &&
rs_table[i].iomem_base == new_mem)) &&
>  
> 
> >2. there seems to be a lack of security considerations for changing the
> >   iomem address.  (ie, changing the iomem address without CAP_SYS_ADMIN.
> >   I added this as an extra check for change_port)
> >
> And this one could be fixed with something like this (this is no a 
> patch, just an idea)
> 
>         change_port = (new_port != ((int) state->port)) ||
>                 (new_serial.hub6 != state->hub6);
> +        change_mem = (new_mem != state->iomem_base)
> 
>         if (!capable(CAP_SYS_ADMIN)) {
> -                if (change_irq || change_port ||
> +                if (change_irq || change_port || change_mem
> 

---------------------------------------------------------------- 
Ed Vance              serial24@macrolink.com
Macrolink, Inc.       1500 N. Kellogg Dr  Anaheim, CA  92807
----------------------------------------------------------------

Re: Serial.c BUG 2.4.x-2.5x

[Roman Kurakin]

Hi,

Russell King wrote:

>On Fri, Mar 01, 2002 at 11:07:03AM -0800, Ed Vance wrote:
>
>>On Fri, Mar 01, 2002 at 4:19 AM, Roman Kurakin wrote:
>>
>>>    Who is responsible person for applying [serial driver] patches 
>>>    to main tree?
>>>
>
>This particular bug has already been fixed in the rewrite, as I originally
>said back on 14 November 2001.
>
I remember this, I thought some one responsible for updating current 
version of the main tree.
Now I see the reason this didn't  get into recent stable versions.

>The patch does fine for the most part, but I have two worries:
>
>1. the possibilities of pushing through changes in the IO or memory space
>   by changing the other space at the same time. (ie, port = 1, iomem =
>   0xfe007c00 and you already have a line at port = 0, iomem = 0xfe007c00).
>   I delt with this properly using the resource management subsystem.
>
I think such code could solve this problem ...

- 			    (rs_table[i].port == new_port) &&
+ 			    ((rs_table[i].port && rs_table[i].port == new_port) ||
+			    ((rs_table[i].iomem_base && rs_table[i].iomem_base == new_mem)) &&
 

>2. there seems to be a lack of security considerations for changing the
>   iomem address.  (ie, changing the iomem address without CAP_SYS_ADMIN.
>   I added this as an extra check for change_port)
>
And this one could be fixed with something like this (this is no a 
patch, just an idea)

        change_port = (new_port != ((int) state->port)) ||
                (new_serial.hub6 != state->hub6);
+        change_mem = (new_mem != state->iomem_base)

        if (!capable(CAP_SYS_ADMIN)) {
-                if (change_irq || change_port ||
+                if (change_irq || change_port || change_mem

As I wrote I didn't check serial.c for all possible problems, I just 
find one bug and suggested
the way it could be solved.

Best regards,
                        Roman Kurakin

>>I then asked Russell to set the rules for this co-ordination and no response
>>has been forthcoming. Perhaps he missed my question?
>>
>
>I have a fair bit of email backed up at the moment, but I have been in
>contact with Ted T'so recently.  I won't say much more at the moment,
>but should have something in a month or two.  Until then I'd rather not
>say too much publically.
>

Re: Serial.c BUG 2.4.x-2.5x

[Russell King]

On Fri, Mar 01, 2002 at 11:07:03AM -0800, Ed Vance wrote:
> On Fri, Mar 01, 2002 at 4:19 AM, Roman Kurakin wrote:
> > 
> >     Who is responsible person for applying [serial driver] patches 
> >     to main tree?

This particular bug has already been fixed in the rewrite, as I originally
said back on 14 November 2001.

The patch does fine for the most part, but I have two worries:

1. the possibilities of pushing through changes in the IO or memory space
   by changing the other space at the same time. (ie, port = 1, iomem =
   0xfe007c00 and you already have a line at port = 0, iomem = 0xfe007c00).
   I delt with this properly using the resource management subsystem.

2. there seems to be a lack of security considerations for changing the
   iomem address.  (ie, changing the iomem address without CAP_SYS_ADMIN.
   I added this as an extra check for change_port)

> I then asked Russell to set the rules for this co-ordination and no response
> has been forthcoming. Perhaps he missed my question?

I have a fair bit of email backed up at the moment, but I have been in
contact with Ted T'so recently.  I won't say much more at the moment,
but should have something in a month or two.  Until then I'd rather not
say too much publically.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

RE: Serial.c BUG 2.4.x-2.5x

[Ed Vance]

On Fri, Mar 01, 2002 at 4:19 AM, Roman Kurakin wrote:
> 
>     Who is responsible person for applying [serial driver] patches 
>     to main tree?

Hi Roman,

Well it's a little complicated. Russell King is the official serial driver
maintainer, at least for 2.5. He is very busy right now on a rewrite of the
serial driver subsystem that will eventually make things better in many ways
for writing support for new devices with serial nature. 

He has (properly) backed away from trying to also support the existing
driver. There has been no willing victim to take on maintainer duties for
the existing driver since Ted Tso got busy with other things over a year
ago. 

BTW, your patch is correct and, as you suspected, there are indeed other
ways that the existing driver is broken for memory mapped devices. My
favorite is the bug that causes kudzu to die with a null pointer dereference
during system initialization IFF there is a memory mapped serial card
present. 

I have been trying to volunteer to ride the current driver into its sunset,
so as to (1) get my own changes in :-), and (2) call for the ignored patches
and help fix the known broken bits (be it known that since 2.4 is a "stable"
release, there are good ideas/enhancements that we simply should not do in
2.4) - without distracting Russell from his good work. The last word I
received was:

> The more help the better as always - providing you can co-ordinate
> with Russell. 

I then asked Russell to set the rules for this co-ordination and no response
has been forthcoming. Perhaps he missed my question? So there is almost, but
not quite, somebody official to evaluate changes to the existing driver and
push the verified patches to Marcelo. I don't think it is time to abandon
the existing driver, just yet. 

BTW, I monitor the abandoned linux-serial mailing list. If you post ignored
patches there I will be more likely to see them than on lkml. If and when
the people in charge say "go", I will start grinding on the existing serial
driver. 

Best regards,
Ed Vance

---------------------------------------------------------------- 
Ed Vance              edv@macrolink.com
Macrolink, Inc.       1500 N. Kellogg Dr  Anaheim, CA  92807
----------------------------------------------------------------

-----Original Message-----
From: Roman Kurakin [mailto:rik@cronyx.ru]
Sent: Friday, March 01, 2002 4:19 AM
To: linux-kernel@vger.kernel.org
Subject: Serial.c BUG 2.4.x-2.5x


Hi,

    Who is responsible person for applying patches to main tree?

This time I decide to send 2.5.5 patch version:

--- serial-255.c    Thu Feb 28 19:24:47 2002
+++ ../serial-255.c    Wed Feb 20 05:10:59 2002
@@ -2084,7 +2084,6 @@
     unsigned int        i,change_irq,change_port;
     int             retval = 0;
     unsigned long        new_port;
-    unsigned long        new_mem;
 
     if (copy_from_user(&new_serial,new_info,sizeof(new_serial)))
         return -EFAULT;
@@ -2094,7 +2093,6 @@
     new_port = new_serial.port;
     if (HIGH_BITS_OFFSET)
         new_port += (unsigned long) new_serial.port_high << 
HIGH_BITS_OFFSET;
-    new_mem = new_serial.iomem_base;
 
     change_irq = new_serial.irq != state->irq;
     change_port = (new_port != ((int) state->port)) ||
@@ -2136,7 +2134,6 @@
         for (i = 0 ; i < NR_PORTS; i++)
             if ((state != &rs_table[i]) &&
                 (rs_table[i].port == new_port) &&
-                (rs_table[i].iomem_base == new_mem) &&
                 rs_table[i].type)
                 return -EADDRINUSE;
     }

-------- Original Message --------
 Subject: Serial.c Bug
 Date: Wed, 14 Nov 2001 13:02:47 +0300
 From: Roman Kurakin <rik@cronyx.ru>
 To: linux-kernel@vger.kernel.org

   I have found a bug. It is in support of serial cards which uses 
memory for
   I/O insted of ports. I made a patch for serial.c and fix one place, but
   probably the problem like this one could be somewhere else.
  
   If you try to use setserial with such cards you will get "Address in use"
   (-EADDRINUSE)
     
   Best regards,
                    Kurakin Roman


Best regards,
                        Roman Kurakin




-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Serial.c BUG 2.4.x-2.5x

[Roman Kurakin]

Hi,

    Who is responsible person for applying patches to main tree?

This time I decide to send 2.5.5 patch version:

--- serial-255.c    Thu Feb 28 19:24:47 2002
+++ ../serial-255.c    Wed Feb 20 05:10:59 2002
@@ -2084,7 +2084,6 @@
     unsigned int        i,change_irq,change_port;
     int             retval = 0;
     unsigned long        new_port;
-    unsigned long        new_mem;
 
     if (copy_from_user(&new_serial,new_info,sizeof(new_serial)))
         return -EFAULT;
@@ -2094,7 +2093,6 @@
     new_port = new_serial.port;
     if (HIGH_BITS_OFFSET)
         new_port += (unsigned long) new_serial.port_high << 
HIGH_BITS_OFFSET;
-    new_mem = new_serial.iomem_base;
 
     change_irq = new_serial.irq != state->irq;
     change_port = (new_port != ((int) state->port)) ||
@@ -2136,7 +2134,6 @@
         for (i = 0 ; i < NR_PORTS; i++)
             if ((state != &rs_table[i]) &&
                 (rs_table[i].port == new_port) &&
-                (rs_table[i].iomem_base == new_mem) &&
                 rs_table[i].type)
                 return -EADDRINUSE;
     }

-------- Original Message --------
 Subject: Serial.c Bug
 Date: Wed, 14 Nov 2001 13:02:47 +0300
 From: Roman Kurakin <rik@cronyx.ru>
 To: linux-kernel@vger.kernel.org

   I have found a bug. It is in support of serial cards which uses 
memory for
   I/O insted of ports. I made a patch for serial.c and fix one place, but
   probably the problem like this one could be somewhere else.
  
   If you try to use setserial with such cards you will get "Address in use"
   (-EADDRINUSE)
     
   Best regards,
                    Kurakin Roman


Best regards,
                        Roman Kurakin