sg_dd bpt= count=

sg_dd bpt= count=

[Pat LaVarre]

> I find some bugs "interesting".

sudo sg_dd of=/dev/sg0 if=/dev/zero bs=2k bpt= count=
may reliably take down kernels.

I say this because I saw 2.6.0-test7 go down twice, but I haven't yet
confirmed my sg utils are the latest available, and with regret I have
to admit just now I have to turn my attention elsewhere for a few days. 
Noise keeps me from being sure if ping survives.  ssh goes down, the cat
/proc/kmsg there showed nothing, the frozen gui of the main console
showed a couple of complaints about unrecognised multipliers.

Your mileage may vary.

Pat LaVarre

Re: sg_dd bpt= count=

[Pat LaVarre]

> > I find some bugs "interesting".
>
> sudo sg_dd of=/dev/sg0 if=/dev/zero bs=2k bpt= count=
> may reliably take down kernels.
>
> I say this because I saw 2.6.0-test7 go down twice,

And now also 2.6.0-test8.  This time ping survived, but again ssh did
not, and Ctrl+Alt+F$n doesn't give me an alternate console.

> my sg utils are the latest available

Confirmed:
http://www.torque.net/sg/p/sg3_utils-1.05.tgz
`cksum` reports: 4285724795 188010 sg3_utils-1.05.tgz

Pat LaVarre

Re: sg_dd bpt= count=

[Pat LaVarre]

> > sudo sg_dd of=/dev/sg0 if=/dev/zero bs=2k bpt= count=
> > may reliably take down kernels.

To sg3_utils sg_dd.c I first propose the following patch, to persuade
get_num to return determinate results more often.

Specifically I propose changing:

        char c;
        res = sscanf(buf, "%d%c", &num, &c);
        if (0 == res) ...
        else if (1 == res) ...
        else {
                switch (c) { ...

Personally I believe that source fragment switches on uninitialised c in
the situation `man sscanf` describes as: "RETURN VALUE ... The value EOF
is returned if an input failure occurs before any conversion such as an
end-of-file occurs ...".

As a test, I did separately execute get_num("").  For me once the
uninitialised c and num were then 8 and 1108545272 (aka x42130EF8), so
the result was -1.  I notice gcc -Wall doesn't mention this kind of
read-before-write.

Pat LaVarre

P.S. Also I wonder if we would prefer rewriting these "return -1" as
loud exits e.g.:

fprintf(stderr, "file %s line %d\n", __FILE__, __LINE__);
exit(-1);

--- sg3_utils-1.05/sg_dd.c	2003-10-19 03:35:32.000000000 -0600
+++ sg3_utils/sg_dd.c	2003-10-20 13:35:20.515143520 -0600
@@ -475,10 +475,10 @@
     char c;
 
     res = sscanf(buf, "%d%c", &num, &c);
-    if (0 == res)
-        return -1;
-    else if (1 == res)
+    if (1 == res)
         return num;
+    else if (2 != res)
+        return -1;
     else {
         switch (c) {
         case 'c':

Re: sg_dd bpt= count=

[Pat LaVarre]

Doug G:

Have we reached closure now?  I also see crashes if I try:

sg_dd of=/dev/sg0 bs=2k bpt=-1

To oops this way, I only need write privileges into some of /dev/sg*,
not the other root privileges.

Therefore I now propose the following patch, a replacement of and an
improvement on my earlier sg3_utils-1.05/sg_dd.c patch.

I got to this patch courtesy some fprintf stderr and one-second sleeps. 
I now think `sg_dd of=/dev/sg0 bs=2k bpt=` can mean, in part:

#include <fcntl.h>
#include <scsi/sg.h>
#include <sys/ioctl.h>
#include <unistd.h>

int main(void)
{
	char const * fn = "/dev/sg0";
	int fd = open(fn, O_RDWR);
	if (0 <= fd) {
		int t = -2048;
		ioctl(fd, SG_SET_RESERVED_SIZE, &t);
	}
	return 0;
}

Compiled separately, that specific example takes down 2.6.0-test8 here,
same as sg_dd does.  Ctrl+Alt+F2 worked long enough for me to die in an
alternate console, but still I died.

SG_SET_RESERVED_SIZE at Google is:
http://www.tldp.org/HOWTO/SCSI-Generic-HOWTO/gs_rs_size.html
but that doesn't tell me what the permissible range of t is.

Nonnegative is my guess of our intent, I see bpt=0 does not so
immediately oops.

Pat LaVarre

diff -u sg3_utils-1.05/sg_dd.c sg3_utils/sg_dd.c
--- sg3_utils-1.05/sg_dd.c	2003-10-19 03:35:32.000000000 -0600
+++ sg3_utils/sg_dd.c	2003-10-20 15:18:06.204065056 -0600
@@ -475,10 +475,10 @@
     char c;
 
     res = sscanf(buf, "%d%c", &num, &c);
-    if (0 == res)
-        return -1;
-    else if (1 == res)
+    if (1 == res)
         return num;
+    else if (2 != res)
+        return -1;
     else {
         switch (c) {
         case 'c':
@@ -621,6 +621,10 @@
         usage();
         return 1;
     }
+    if (bpt < 0) {
+        fprintf(stderr, "bpt cannot be negative\n");
+        return 1;
+    }
     if ((skip < 0) || (seek < 0)) {
         fprintf(stderr, "skip and seek cannot be negative\n");
         return 1;

Re: sg_dd bpt= count=

[Douglas Gilbert]

Pat LaVarre wrote:
>>>sudo sg_dd of=/dev/sg0 if=/dev/zero bs=2k bpt= count=
>>>may reliably take down kernels.
> 
> 
> To sg3_utils sg_dd.c I first propose the following patch, to persuade
> get_num to return determinate results more often.
> 
> Specifically I propose changing:
> 
>         char c;
>         res = sscanf(buf, "%d%c", &num, &c);
>         if (0 == res) ...
>         else if (1 == res) ...
>         else {
>                 switch (c) { ...
> 
> Personally I believe that source fragment switches on uninitialised c in
> the situation `man sscanf` describes as: "RETURN VALUE ... The value EOF
> is returned if an input failure occurs before any conversion such as an
> end-of-file occurs ...".
> 
> As a test, I did separately execute get_num("").  For me once the
> uninitialised c and num were then 8 and 1108545272 (aka x42130EF8), so
> the result was -1.  I notice gcc -Wall doesn't mention this kind of
> read-before-write.
> 
> Pat LaVarre
> 
> P.S. Also I wonder if we would prefer rewriting these "return -1" as
> loud exits e.g.:
> 
> fprintf(stderr, "file %s line %d\n", __FILE__, __LINE__);
> exit(-1);
> 
> --- sg3_utils-1.05/sg_dd.c	2003-10-19 03:35:32.000000000 -0600
> +++ sg3_utils/sg_dd.c	2003-10-20 13:35:20.515143520 -0600
> @@ -475,10 +475,10 @@
>      char c;
>  
>      res = sscanf(buf, "%d%c", &num, &c);
> -    if (0 == res)
> -        return -1;
> -    else if (1 == res)
> +    if (1 == res)
>          return num;
> +    else if (2 != res)
> +        return -1;
>      else {
>          switch (c) {
>          case 'c':

Pat,
Applied to sg3_utils. Also added a check for non-positive bpt.
There is a new beta on http://www.torque.net/sg

Doug Gilbert

Re: sg_dd bpt= count=

[Pat LaVarre]

> Applied to sg3_utils. Also added a check for non-positive bpt.
> There is a new beta on http://www.torque.net/sg

Perfect, thanks.

Pat LaVarre

P.S. More specifically:

In a Linux built without linux-scsi "[PATCH] SG_SET_RESERVED_SIZE
negative oops", now rather than oops I see:

$ sg_dd of=/dev/sg0 if=/dev/zero bs=2k bpt= count=
bpt must be greater than 0
$

Also: diff -Nur sg3_utils-1.05-was sg3_utils-1.05
tells me:
+  - sg_dd, sgp_dd, sgm_dd, sg_read, sg_turs: require bpt > 0