On Sat, 19 Oct 2002, Edouard Gomez wrote:
>
> Hello,
>
(I've confused myself :-(. These are comments on the patch from
r1.24 to r1.25 of pppoa3.c. I'll reply with my comments on your
suggested patch in another email. (I hadn't read your email
properly and assumed that the latest change in cvs was the patch :-(
@@ -268,13 +272,11 @@
};
/* Reading and writing buffers */
-#define BUFFER_SIZE (64*1024)
-#define HDLC_HEADER_SIZE 2
-
-static unsigned char ppp_read_buf[HDLC_HEADER_SIZE + 1367*ATM_CELL_TOTAL_SIZE];
-static unsigned char ppp_write_buf[HDLC_HEADER_SIZE + 1367*ATM_CELL_TOTAL_SIZE];
-static unsigned char *aal5_send_buf = &ppp_read_buf[HDLC_HEADER_SIZE];
-static unsigned char *aal5_recv_buf = &ppp_write_buf[HDLC_HEADER_SIZE];
+#define HDLC_HEADER_SIZE 2
+#define BRIDGING1483_HEADER 12
+#define AAL5_MAX_SIZE 64*1024
+#define BUFFER_SIZE HDLC_HEADER_SIZE + BRIDGING1483_HEADER + \
+ 1367*ATM_CELL_TOTAL_SIZE
These last two (and especially the BUFFER_SIZE) ought to be enclosed with "("
and ")".
I can imagine a change later
/* Use double buffer so we can process one packet on one cpu while receiving
another with the other cpu */
unsigned char read_buffer[BUFFER_SIZE * 2];
which might not crash straight away!
@@ -751,19 +759,26 @@
sigfillset(&signal_set);
pthread_sigmask(SIG_SETMASK, &signal_set, NULL);
+ /* Allocate buffer memory */
+ if((buffer = malloc(BUFFER_SIZE)) == NULL)
+ goto local_end;
<snip source_buf = buffer stuff>
/* Reads data from ppp(d) */
- n = ppp_read(STDIN_FILENO, ppp_read_buf, BUFFER_SIZE);
+ n = read_source(fdin, source_buf, AAL5_MAX_SIZE);
It isn't immediately obvious that AAL5_MAX_SIZE <= BUFFER_SIZE
At the very least there should be a
#if AAL5_MAX_SIZE > BUFFER_SIZE
#error "AAL5_MAX_SIZE must not be bigger than BUFFER_SIZE"
#endif
There is a similar thing in read_from_usb_thread but its more obviously
correct (53*64)<=(BUFFER_SIZE) :-)
Finally, can you apply (or comment upon) this patch which is required to
fix a number of potential SIGSEGV problems that could affect anybody
(although, to the best of my knowledge only one person has experienced
one). In particular, if the last cell of a frame is a management cell then
you will get a core dump currently.
The real_length<length test isn't needed to prevent a crash but there
is a possible subtle attack sending malformed ATM packets which would
enable you to read other traffic from the buffers in pppoa3. (It is
relatively trivial to generate these packets although I don't know
if or where they would be filtered or whether all ISPs would filter
them the same way)
I think that the pti=0 lines fix the need for the length<6 line so
you could possibly leave this one out if you think performance is
critical. However, as we are going to do an expensive CRC calculation
this isn't going to make a measurable difference, indeed on the
pentium I wouldn't be surprised if it took zero time as it could
run in parallel with the code setting up the parameters for
the call to aal5_calc_crc which is going to involve memory fetches
and probably cause pipeline stalls.
Index: atm.c
===================================================================
RCS file: /cvsroot/speedtouch/speedtouch/src/atm.c,v
retrieving revision 1.6
diff -u -r1.6 atm.c
--- atm.c 21 Mar 2002 23:35:43 -0000 1.6
+++ atm.c 20 Oct 2002 15:45:01 -0000
@@ -329,6 +329,7 @@
atm_header_get_pti(src));
src += ATM_CELL_TOTAL_SIZE;
length -= ATM_CELL_TOTAL_SIZE;
+ pti = 0; /* Reset pti because this could be the last cell
+in the buffer */
continue;
}
@@ -343,6 +344,7 @@
report(0, REPORT_DEBUG|REPORT_DATE|REPORT_DUMP, "Management
cell in stream (OAM)\n", src, ATM_CELL_TOTAL_SIZE);
src += ATM_CELL_TOTAL_SIZE;
length -= ATM_CELL_TOTAL_SIZE;
+ pti = 0; /* Reset pti because this could be the last cell
+in the buffer */
continue;
}
@@ -384,6 +386,8 @@
int real_length;
unsigned int frame_crc, computed_crc;
+ if(length < 6)
+ return(-1);
/* CRC checking */
computed_crc = ~aal5_calc_crc(frame, length - 4, ~0);
@@ -395,6 +399,10 @@
/* Find the real len */
real_length = (((int)frame[length - 6])<<8)|((int)frame[length - 5]);
+
+ if(real_length > length)
+ /* This is most likely a deliberate hack attempt - perhaps we ought to
+log this??? */
+ return(-1);
/* Copy the data */
if(data != frame)
Index: pppoa3.c
===================================================================
RCS file: /cvsroot/speedtouch/speedtouch/src/pppoa3.c,v
retrieving revision 1.25
diff -u -r1.25 pppoa3.c
--- pppoa3.c 13 Oct 2002 20:42:12 -0000 1.25
+++ pppoa3.c 20 Oct 2002 15:45:04 -0000
@@ -894,6 +894,7 @@
/* A buffer overflow has been detected */
if(pti<0) {
report(0, REPORT_ERROR|REPORT_DATE, "Buffer overflow, too many
cells for the same aal5 frame\n");
+ pti = 0;
}
/* As the last pti is 1, we have to send the aal5_frame data */
--
God said, "div D = rho, div B = 0, curl E = - @B/@t, curl H = J + @D/@t,"
and there was light.
http://tjw.hn.org/ http://www.locofungus.btinternet.co.uk/
Liste de diffusion modem ALCATEL SpeedTouch USB
Pour se désinscrire : mailto:speedtouch-request@;ml.free.fr?subject=unsubscribe
