Re: array underflow in receive_SyncParam()?

[Philipp Reisner]

Am Dienstag, 27. März 2012, 10:10:36 schrieb Dan Carpenter:
> I had a question about the following code:
> 
> drivers/block/drbd/drbd_receiver.c
>   2808                  if (apv == 88) {
>   2809                          if (data_size > SHARED_SECRET_MAX) {
>   2810                                  dev_err(DEV, "verify-alg too long, "
> 2811                                      "peer wants %u, accepting only %u
> byte\n", 2812                                                  data_size,
> SHARED_SECRET_MAX); 2813                                  return false;
>   2814                          }
>   2815
>   2816                          if (drbd_recv(mdev, p->verify_alg,
> data_size) != data_size) 2817                                  return
> false;
>   2818
>   2819                          /* we expect NUL terminated string */
>   2820                          /* but just in case someone tries to be evil
> */ 2821                          D_ASSERT(p->verify_alg[data_size-1] == 0);
> 2822                          p->verify_alg[data_size-1] = 0;
>                                               ^^^^^^^^^
> Is it possible for data_size to be zero here leading to an array
> underflow?  We test for overflows, but I don't see any place where we
> test for zero.
> 

Hi Dan,

You are right, we are relying on the fact that DRBD peers that
use the protocol 88 send a positive data_size (what they do).

But if we consider a modified peer, then this is a bug. Suggesting
the following patch:

diff --git a/drbd/drbd_receiver.c b/drbd/drbd_receiver.c
index 3ef6130..317d307 100644
--- a/drbd/drbd_receiver.c
+++ b/drbd/drbd_receiver.c
@@ -3086,9 +3086,9 @@ STATIC int receive_SyncParam(struct drbd_conf *mdev, 
enum drbd_packets cmd, unsi
 
        if (apv >= 88) {
                if (apv == 88) {
-                       if (data_size > SHARED_SECRET_MAX) {
-                               dev_err(DEV, "verify-alg too long, "
-                                   "peer wants %u, accepting only %u byte\n",
+                       if (data_size > SHARED_SECRET_MAX || data_size == 0) {
+                               dev_err(DEV, "verify-alg of wrong size, "
+                                       "peer wants %u, accepting only %u 
byte\n",
                                                data_size, SHARED_SECRET_MAX);
                                return false;
                        }

Best,
 Phil
-- 
: Dipl-Ing Philipp Reisner
: LINBIT | Your Way to High Availability
: Tel: +43-1-8178292-50, Fax: +43-1-8178292-82
: http://www.linbit.com

DRBD(R) and LINBIT(R) are registered trademarks of LINBIT, Austria.

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