eicon: make buffer larger

PATCH Applied Dan Carpenter 4 participants 10 messages Oct 4, 2010 – Oct 7, 2010

Subsystems (3)

Networking Network drivers Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2010-10-04 19:24 UTC #0 lore raw

PATCH 1/1 eicon: make buffer larger

In diva_mnt_add_xdi_adapter() we do this:
strcpy (clients[id].drvName, tmp);
strcpy (clients[id].Dbg.drvName, tmp);

The "clients[id].drvName" is a 128 character buffer and "clients[id].Dbg.drvName" was originally a 16 character buffer but I've changed it to 128 as well. We don't actually use 128 characters but we do use more than 16.

I've also changed the size of "tmp" to 128 characters instead of 256.

Signed-off-by Dan Carpenter <[email protected]>

drivers/isdn/hardware/eicon/debuglib.h +1 -1

index 8ea5877..02eed6b 100644
@@ -249,7 +249,7 @@ typedef struct _DbgHandle_
  }     regTime ;  /* timestamp for registration       */
  void               *pIrp ;   /* ptr to pending i/o request       */
  unsigned long       dbgMask ;  /* current debug mask               */
- char                drvName[16] ; /* ASCII name of registered driver  */
+ char                drvName[128] ; /* ASCII name of registered driver  */
  char                drvTag[64] ; /* revision string     */
  DbgEnd              dbg_end ;  /* function for debug closing       */
  DbgLog              dbg_prt ;  /* function for debug appending     */

drivers/isdn/hardware/eicon/debug.c +1 -1

index 33ce89e..3626401 100644
@@ -862,7 +862,7 @@ void diva_mnt_add_xdi_adapter (const DESCRIPTOR* d) {
   diva_os_spin_lock_magic_t old_irql, old_irql1;
   dword sec, usec, logical, serial, org_mask;
   int id, best_id = 0, free_id = -1;
-  char tmp[256];
+  char tmp[128];
   diva_dbg_entry_head_t* pmsg = NULL;
   int len;
   word size;

Armin Schindler

2010-10-06 07:25 UTC #1 lore raw

Quoted 8 lines · Dan Carpenter

In diva_mnt_add_xdi_adapter() we do this:
strcpy (clients[id].drvName, tmp);
strcpy (clients[id].Dbg.drvName, tmp);
The "clients[id].drvName" is a 128 character buffer and "clients[id].Dbg.drvName" was originally a 16 character buffer but I've changed it to 128 as well. We don't actually use 128 characters but we do use more than 16.

I don't see any reason for that change. The driver names here do not use more than 16 characters and when filled, the length is checked anyway. Please avoid changing the size of that structure.

Armin

Quoted 31 lines

I've also changed the size of "tmp" to 128 characters instead of 256.

Signed-off-by Dan Carpenter <[email protected]>

drivers/isdn/hardware/eicon/debuglib.h +1 -1

index 8ea5877..02eed6b 100644
@@ -249,7 +249,7 @@ typedef struct _DbgHandle_
 }     regTime ;  /* timestamp for registration       */
 void               *pIrp ;   /* ptr to pending i/o request       */
 unsigned long       dbgMask ;  /* current debug mask               */
- char                drvName[16] ; /* ASCII name of registered driver  */
+ char                drvName[128] ; /* ASCII name of registered driver  */
 char                drvTag[64] ; /* revision string     */
 DbgEnd              dbg_end ;  /* function for debug closing       */
 DbgLog              dbg_prt ;  /* function for debug appending     */

drivers/isdn/hardware/eicon/debug.c +1 -1

index 33ce89e..3626401 100644
@@ -862,7 +862,7 @@ void diva_mnt_add_xdi_adapter (const DESCRIPTOR* d) {
  diva_os_spin_lock_magic_t old_irql, old_irql1;
  dword sec, usec, logical, serial, org_mask;
  int id, best_id = 0, free_id = -1;
-  char tmp[256];
+  char tmp[128];
  diva_dbg_entry_head_t* pmsg = NULL;
  int len;
  word size;

Dan Carpenter

author

2010-10-06 07:47 UTC #2 lore raw

Quoted 5 lines · Armin Schindler

Quoted 4 lines · Dan Carpenter
In diva_mnt_add_xdi_adapter() we do this:
strcpy (clients[id].drvName, tmp);
strcpy (clients[id].Dbg.drvName, tmp);

Quoted 9 lines

Quoted 4 lines
The "clients[id].drvName" is a 128 character buffer and "clients[id].Dbg.drvName" was originally a 16 character buffer but I've changed it to 128 as well. We don't actually use 128 characters but we do use more than 16.
I don't see any reason for that change. The driver names here do not use more than 16 characters and when filled, the length is checked anyway. Please avoid changing the size of that structure.

drivers/isdn/hardware/eicon/debug.c diva_mnt_add_xdi_adapter()
874 sprintf (tmp, "ADAPTER:%d SN:%u-%d",
12345678 90123 45 67

That's a minimum 17 characters.

   875               (int)logical,
   876               serial & 0x00ffffff,
   877               (byte)(((serial & 0xff000000) >> 24) + 1));

878 } else {
879 sprintf (tmp, "ADAPTER:%d SN:%u", (int)logical, serial);
880 }

regards,
dan carpenter

Quoted 1 line

Armin

walter harms

2010-10-06 08:16 UTC #3 lore raw

Quoted 19 lines · Dan Carpenter

Quoted 5 lines · Armin Schindler
Quoted 4 lines · Dan Carpenter
In diva_mnt_add_xdi_adapter() we do this:
strcpy (clients[id].drvName, tmp);
strcpy (clients[id].Dbg.drvName, tmp);
Quoted 8 lines
Quoted 4 lines
The "clients[id].drvName" is a 128 character buffer and "clients[id].Dbg.drvName" was originally a 16 character buffer but I've changed it to 128 as well. We don't actually use 128 characters but we do use more than 16.
I don't see any reason for that change. The driver names here do not use more than 16 characters and when filled, the length is checked anyway. Please avoid changing the size of that structure.
drivers/isdn/hardware/eicon/debug.c diva_mnt_add_xdi_adapter()
874 sprintf (tmp, "ADAPTER:%d SN:%u-%d",
12345678 90123 45 67

Quoted 9 lines

That's a minimum 17 characters.

   875               (int)logical,
   876               serial & 0x00ffffff,
   877               (byte)(((serial & 0xff000000) >> 24) + 1));
   878    } else {
   879      sprintf (tmp, "ADAPTER:%d SN:%u", (int)logical, serial);
   880    }

for that reason i am a fan of kasprintf().
Maybe this a solution here also ? samething like:

kasprintf (&buf, "ADAPTER:%d SN:%u-%d",12345678,90123,45,67):
...
kstrncpy(tmp,buf,sizeof(buf));
....
free(buf);

that would keep overflows away until the changes in the structure are ready.

re,
wh

Armin Schindler

2010-10-06 08:21 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 20 lines · Dan Carpenter

Quoted 5 lines · Armin Schindler
Quoted 4 lines · Dan Carpenter
In diva_mnt_add_xdi_adapter() we do this:
strcpy (clients[id].drvName, tmp);
strcpy (clients[id].Dbg.drvName, tmp);
Quoted 9 lines
Quoted 4 lines
The "clients[id].drvName" is a 128 character buffer and "clients[id].Dbg.drvName" was originally a 16 character buffer but I've changed it to 128 as well. We don't actually use 128 characters but we do use more than 16.
I don't see any reason for that change. The driver names here do not use more than 16 characters and when filled, the length is checked anyway. Please avoid changing the size of that structure.
drivers/isdn/hardware/eicon/debug.c diva_mnt_add_xdi_adapter()
874 sprintf (tmp, "ADAPTER:%d SN:%u-%d",
12345678 90123 45 67

Quoted 8 lines

That's a minimum 17 characters.
  875               (int)logical,
  876               serial & 0x00ffffff,
  877               (byte)(((serial & 0xff000000) >> 24) + 1));
878 } else {
879 sprintf (tmp, "ADAPTER:%d SN:%u", (int)logical, serial);
880 }

this is tmp with a bigger size. It seems you are mixing the sizes of drvName and tmp.

Armin

Dan Carpenter

author

2010-10-06 08:31 UTC #5 lore raw

Quoted 30 lines · Armin Schindler

Quoted 20 lines · Dan Carpenter
Quoted 5 lines · Armin Schindler
Quoted 4 lines · Dan Carpenter
In diva_mnt_add_xdi_adapter() we do this:
strcpy (clients[id].drvName, tmp);
strcpy (clients[id].Dbg.drvName, tmp);
Quoted 9 lines
Quoted 4 lines
The "clients[id].drvName" is a 128 character buffer and "clients[id].Dbg.drvName" was originally a 16 character buffer but I've changed it to 128 as well. We don't actually use 128 characters but we do use more than 16.
I don't see any reason for that change. The driver names here do not use more than 16 characters and when filled, the length is checked anyway. Please avoid changing the size of that structure.
drivers/isdn/hardware/eicon/debug.c diva_mnt_add_xdi_adapter()
874 sprintf (tmp, "ADAPTER:%d SN:%u-%d",
12345678 90123 45 67
Quoted 8 lines
That's a minimum 17 characters.
  875               (int)logical,
  876               serial & 0x00ffffff,
  877               (byte)(((serial & 0xff000000) >> 24) + 1));
878 } else {
879 sprintf (tmp, "ADAPTER:%d SN:%u", (int)logical, serial);
880 }

Quoted 3 lines

this is tmp with a bigger size. It seems you are mixing the sizes of drvName and tmp.

What I mean is that later on we use strcpy() to copy "tmp" into "clients[id].Dbg.drvName"

   927    strcpy (clients[id].drvName,     tmp);
   928    strcpy (clients[id].Dbg.drvName, tmp);
                               ^
                    this buffer is only 16 chars

regards,
dan carpenter

Quoted 1 line

Armin

Armin Schindler

2010-10-06 08:44 UTC #6 lore raw

Quoted 35 lines · Dan Carpenter

Quoted 30 lines · Armin Schindler
Quoted 20 lines · Dan Carpenter
Quoted 5 lines · Armin Schindler
Quoted 4 lines · Dan Carpenter
In diva_mnt_add_xdi_adapter() we do this:
strcpy (clients[id].drvName, tmp);
strcpy (clients[id].Dbg.drvName, tmp);
Quoted 9 lines
Quoted 4 lines
The "clients[id].drvName" is a 128 character buffer and "clients[id].Dbg.drvName" was originally a 16 character buffer but I've changed it to 128 as well. We don't actually use 128 characters but we do use more than 16.
I don't see any reason for that change. The driver names here do not use more than 16 characters and when filled, the length is checked anyway. Please avoid changing the size of that structure.
drivers/isdn/hardware/eicon/debug.c diva_mnt_add_xdi_adapter()
874 sprintf (tmp, "ADAPTER:%d SN:%u-%d",
12345678 90123 45 67
Quoted 8 lines
That's a minimum 17 characters.
  875               (int)logical,
  876               serial & 0x00ffffff,
  877               (byte)(((serial & 0xff000000) >> 24) + 1));
878 } else {
879 sprintf (tmp, "ADAPTER:%d SN:%u", (int)logical, serial);
880 }
Quoted 3 lines
this is tmp with a bigger size. It seems you are mixing the sizes of drvName and tmp.

Quoted 7 lines

What I mean is that later on we use strcpy() to copy "tmp" into "clients[id].Dbg.drvName"
  927    strcpy (clients[id].drvName,     tmp);
  928    strcpy (clients[id].Dbg.drvName, tmp);
                              ^
                   this buffer is only 16 chars

Now I understand. You are right. So the fix would be to change these strcpy() to e.g. strncpy() or similar.

Armin

Dan Carpenter

author

2010-10-06 12:11 UTC #7 lore raw

Quoted 8 lines · Armin Schindler

Quoted 4 lines
  927    strcpy (clients[id].drvName,     tmp);
  928    strcpy (clients[id].Dbg.drvName, tmp);
                              ^
                   this buffer is only 16 chars
Now I understand. You are right. So the fix would be to change these strcpy() to e.g. strncpy() or similar.

We need more than 16 characters to store the information. What is the problem with just making the buffer larger?

regards,
dan carpenter

Quoted 1 line

Armin

Armin Schindler

2010-10-06 12:49 UTC #8 lore raw

Quoted 6 lines · Dan Carpenter

Quoted 5 lines · Armin Schindler

Quoted 4 lines

  927    strcpy (clients[id].drvName,     tmp);
  928    strcpy (clients[id].Dbg.drvName, tmp);
                              ^
                   this buffer is only 16 chars

Quoted 6 lines

Quoted 3 lines
Now I understand. You are right. So the fix would be to change these strcpy() to e.g. strncpy() or similar.
We need more than 16 characters to store the information. What is the problem with just making the buffer larger?

The larger buffer is not really needed. Later versions of the driver will
use the strcpy/memcpy with checked size anyway.
I just want to avoid a change in the debug structure which is used in more than one module/tool.

But it shouldn't be a real problem.

Armin

David Miller

maintainer Applied

2010-10-07 06:30 UTC #9 lore raw

In reply to Dan Carpenter

From: Dan Carpenter <[email protected]>
Date: Mon, 4 Oct 2010 21:24:59 +0200

Quoted 4 lines

In diva_mnt_add_xdi_adapter() we do this:
strcpy (clients[id].drvName, tmp);
strcpy (clients[id].Dbg.drvName, tmp);

Quoted 8 lines

The "clients[id].drvName" is a 128 character buffer and "clients[id].Dbg.drvName" was originally a 16 character buffer but I've changed it to 128 as well. We don't actually use 128 characters but we do use more than 16.
I've also changed the size of "tmp" to 128 characters instead of 256.
Signed-off-by Dan Carpenter <[email protected]>

Applied.