linux-kernel - Re: [PATCH] [SCSI] arcmsr: &/| confusion in arcmsr_build

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]

Date:	Sat, 1 Jan 2011 10:08:54 -0700
From:	Matthew Wilcox <matthew@....cx>
To:	roel kluin <roel.kluin@...il.com>
Cc:	nick.cheng@...ca.com.tw, Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>, James.Bottomley@...e.de,
	linux-scsi@...r.kernel.org
Subject: Re: [PATCH] [SCSI] arcmsr: &/| confusion in arcmsr_build_ccb()

On Sat, Jan 01, 2011 at 05:45:11PM +0100, roel kluin wrote:
> The WRITE_{6,10,12} are defined numbers, so the branch was always taken.

You're right, but even this will basically always set the write flag,
since some bits will be set the same, even for reads.  Eg:

#define READ_10               0x28
#define WRITE_10              0x2a

It should instead be:

> +	if (pcmd->cmnd[0] == WRITE_6 || pcmd->cmnd[0] == WRITE_10 ||
> +			pcmd->cmnd[0] == WRITE_12 ){

That will of course miss other commands which do writes, such as UNMAP
and WRITE_32.  So we should do it properly:

	if (pcmd->sc_data_direction == DMA_TO_DEVICE)
		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;

I don't have an Areca card to test with.

> Signed-off-by: Roel Kluin <roel.kluin@...il.com>
> ---
>  drivers/scsi/arcmsr/arcmsr_hba.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> See include/scsi/scsi.h:58-123 for
> #define WRITE_6               0x0a
> #define WRITE_10              0x2a
> #define WRITE_12              0xaa
> 
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 17e3df4..a6e5222 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -1171,7 +1171,8 @@ static int arcmsr_build_ccb(struct AdapterControlBlock *acb,
>  	arcmsr_cdb->msgPages = arccdbsize/0x100 + (arccdbsize % 0x100 ? 1 : 0);
>  	if ( arccdbsize > 256)
>  		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_SGL_BSIZE;
> -	if (pcmd->cmnd[0]|WRITE_6 || pcmd->cmnd[0]|WRITE_10 || pcmd->cmnd[0]|WRITE_12 ){
> +	if (pcmd->cmnd[0] & WRITE_6 || pcmd->cmnd[0] & WRITE_10 ||
> +			pcmd->cmnd[0] & WRITE_12 ){
>  		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;
>  	}
>  	ccb->arc_cdb_size = arccdbsize;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/