linux-kernel - Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201701302252.13124.linux@rainbow-software.org>
Date:   Mon, 30 Jan 2017 22:52:12 +0100
From:   Ondrej Zary <linux@...nbow-software.org>
To:     Finn Thain <fthain@...egraphics.com.au>
Cc:     "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Michael Schmitz <schmitzmic@...il.com>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

On Sunday 29 January 2017 02:05:02 Finn Thain wrote:
> On Sat, 28 Jan 2017, Ondrej Zary wrote:
> > On Monday 16 January 2017 00:50:57 Finn Thain wrote:
> > > This series removes some unused code and related comments, addresses
> > > the warnings generated by 'make W=1' and 'make C=1' and fixes a
> > > theoretical bug in the bus reset method in atari_scsi.
> > >
> > > There's also a patch to add a missing error check during target
> > > selection. The only target I tested was a QUANTUM DAYTONA514S disk as
> > > that's all I have access to right now. Some testing with other targets
> > > would be prudent.
> > >
> > > Michael, Ondrej, can I get you to review/test please?
> >
> > Tested on HP C2502 (53C400A chip), Canon FG2-5202 (53C400 chip),
> > DTC-3181L (DTCT-436P chip) and MS-PNR (53C400A chip) ISA cards -
> > everything works fine!
> >
> > Targets tested:
> > QUANTUM  LP240S GM240S01X
> > IBM      DORS-32160
> > IBM      0663L12
> >
> > Thanks.
> >
> > Tested-by: Ondrej Zary <linux@...nbow-software.org>
>
> Very helpful. Thank you, Ondrej.

Also tested two CD-ROM drives and they didn't work (machine hangs). They
didn't work before and looks like they never worked with PDMA.

The fundamental problem of PDMA is that it either transfers all required data
or it breaks horribly. Even when I added timeouts to the possibly infinite
loops (to avoid hangs), the chip remained in some bad state. Sometimes the
53C80 gated IRQ check triggers. This can be hopefully fixed but there is a HW
limitation: if less than 128 bytes were read from SCSI device, they get lost
in chip buffer (the 128B buffers don't swap until full).

Fortunately, most of the requests for too much data are bogus. The problem is
that generic_NCR5380_dma_xfer_len() uses cmd->transfersize which seems to be
wrong. All other NCR5380 drivers use cmd->SCp.this_residual.

This is also why rescan-scsi-bus hangs (cmd->transfersize is 8192 but
cmd->SCp.this_residual is only 96).

This quick fix allows CD-ROM and also rescan-scsi-bus to work.
But it's not complete. Seems that we need:
 - fix pread and pwrite to terminate gracefully
 - something like atari_scsi_dma_xfer_len to allow DMA only for block commands

@@ -588,22 +619,19 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
                                         struct scsi_cmnd *cmd)
 {
-       int transfersize = cmd->transfersize;
+       int transfersize = cmd->SCp.this_residual;

        if (hostdata->flags & FLAG_NO_PSEUDO_DMA)
                return 0;

-       /* Limit transfers to 32K, for xx400 & xx406
-        * pseudoDMA that transfers in 128 bytes blocks.
-        */
-       if (transfersize > 32 * 1024 && cmd->SCp.this_residual &&
-           !(cmd->SCp.this_residual % transfersize))
-               transfersize = 32 * 1024;
-
        /* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */
        if (transfersize % 128)
                transfersize = 0;

+       /* Limit transfers to 32K */
+       if (transfersize > 32 * 1024)
+               transfersize = 32 * 1024;
+
        return transfersize;
 }




-- 
Ondrej Zary