linux-kernel - Re: [PATCH 1/2] mtd: spi-nor: sfdp: save a copy of the SFDP data

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:   Mon, 22 Mar 2021 23:31:39 +0100
From:   Michael Walle <michael@...le.cc>
To:     Pratyush Yadav <p.yadav@...com>
Cc:     linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
        Tudor Ambarus <tudor.ambarus@...rochip.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>
Subject: Re: [PATCH 1/2] mtd: spi-nor: sfdp: save a copy of the SFDP data

Am 2021-03-22 19:42, schrieb Pratyush Yadav:
> On 22/03/21 04:32PM, Michael Walle wrote:
>> Am 2021-03-22 15:21, schrieb Pratyush Yadav:
>> > On 18/03/21 10:24AM, Michael Walle wrote:
>> > > +
>> > > +	sfdp->num_dwords = DIV_ROUND_UP(sfdp_size, sizeof(*sfdp->dwords));
>> >
>> > The SFDP spec says that Parameter Table Pointer should be DWORD aligned
>> > and Parameter Table length is specified in number of DWORDs. So,
>> > sfdp_size should always be a multiple of 4. Any SFDP table where this is
>> > not true is an invalid one.
>> >
>> > Also, the spec says "Device behavior when the Read SFDP command crosses
>> > the SFDP structure boundary is not defined".
>> >
>> > So I think this should be a check for alignment instead of a round-up.
>> 
>> Well, that woundn't help for debugging. I.e. you also want the SFDP 
>> data
>> in cases like this. IMHO we should try hard enough to actually get a
>> reasonable dump.
>> 
>> OTOH we also rely on the header and the pointers in the header. Any
>> other ideas, but just to chicken out?
> 
> Honestly, I don't think reading past the SFDP boundary would be too 
> bad.
> It probably will just be some garbage data. But if you want to avoid
> that, you can always round down instead of up.

Like I said, while the storage will be rounded up to a multiple of
DWORDs, only sfdp_size is transferred. Thus it case a pointer is not
DWORD aligned, we end up with zeros at the end.

I'll add a comment.

> This way you will only
> miss the last DWORD at most. In either case, a warning should be 
> printed
> so this problem can be brought to the user's attention.

I was about to add a warning/debug message. But its the wrong place.
It should really be checked in the for loop which iterates over the
headers before parsing them. You could check sfdp_size but then two
unaligned param pointers might cancel each other out.

This can be a seperate patch, besides adding a warning, should there
be any other things to do, e.g. stop parsing and error out?

..

>> > > +		goto exit;
>> > > +	}
>> > > +
>> > > +	err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sfdp_size, sfdp->dwords);

Btw, this can be spi_nor_read_sfdp(). But I'm not sure, what this
whole dma capable buffer should be. Is kmalloc(GFP_KERNEL)
considered DMA safe?

The buffer ends in spi_nor_read_data(), which is also called from
mtdcore:

spi_nor_read_sfdp()
   spi_nor_read_raw()
     spi_nor_read_data()

mtd_read()
   mtd_read_oob()
     mtd_read_oob_std()
       spi_nor_read()
         spi_nor_read_data()

Is the buffer passed from mtd_read() also DMA-safe? Doesn't the SPI
drivers allocate DMA safe buffers if they need them?

-michael