On Fri, 24 May 2002, Frank C. Earl wrote:
> On Thursday 23 May 2002 04:37 pm, Leif Delgass wrote:
>
> > I've committed code to read BM_GUI_TABLE to reclaim processed buffers and
> > disabled frame and buffer aging with the pattern registers. I've disabled
> > saving/restoring the pattern registers in the DDX and moved the wait for
> > idle to the XAA Sync. This fixes the slowdown on mouse moves. I also
> > fixed a bug in getting the ring head. One bug that remains is that when
> > starting tuxracer or quake (and possibly other apps) from a fresh restart
> > of X, there is a problem where old bits of the back- or frame-buffer show
> > through. With tuxracer (windowed), if I move the window, the problem goes
> > away. It seems that some initial state is not being set correctly or
> > clears aren't working. If I run glxgears (or even tunnel, which uses
> > textures) first, after starting X and before starting another app, the
> > problem isn't there. If someone has a cvs build or binary from before
> > Monday the 20th but after Saturday the 18th, could you test to see if this
> > happens? I'm not sure if this is new behavior or not.
> >
> > I tried removing the flush on swaps in my tree and things seem to still
> > work fine (the bug mentioned above is still there, however). We may need
> > to think of an alternate way to do frame aging and throttling, without
> > using a pattern register.
>
> I've been pondering the code you've done (not the latest commited, but what
> was described to me a couple of weeks back...) how do you account for
> securing the BM_GUI_TABLE check and the pattern register aging in light of
> the engine being able to write to most all registers? It occured to me that
> there's a potential security risk (allowing malicious clients to possibly
> confuse/hang the engine) with the design described to me back a little while
> ago.
Well, I just went back and looked at Jose's test for writing
BM_GUI_TABLE_CMD from within a buffer and realized that it had a bug.
The register addresses weren't converted to MM offsets. So I fixed that
and ran the test. With two descriptors, writing BM_GUI_TABLE_CMD does not
cause the second descriptor to be read from the new address, but
BM_GUI_TABLE reads back with the new address written in the first buffer
at the end of the test. Then I tried setting up three descriptors, and lo
and behold, after processing the first two descriptors, the engine
switches to the new table address written in the first buffer! I think
it's because of the pre-incrementing (prefetching?) of BM_GUI_TABLE that
there's a delay of one descriptor, but it IS possible to derail a bus
master in progress and set it processing from a different table in
mid-stream. Plus, if the address is bogus or the table is misconstructed,
this will cause an engine lockup and take out DMA until the machine is
cold restarted. The code for the test I used is attached.
So it would appear that allowing clients to add register commands to a
buffer without verifying them is _not_ secure. This is going to make
things harder, especially for vertex buffers. This is going to require
copying buffers and adding commands or unmapping and verifying
client-submitted buffers in the DRM. I'd like to continue on the path
we're on until we can get DMA running smoothly and then we'll have to come
back and fix this problem.
--
Leif Delgass
http://www.retinalburn.net
static int mach64_bm_dma_test2( drm_device_t *dev )
{
drm_mach64_private_t *dev_priv = dev->dev_private;
dma_addr_t data_handle, data2_handle, table2_handle;
void *cpu_addr_data, *cpu_addr_data2, *cpu_addr_table2;
u32 data_addr, data2_addr, table2_addr;
u32 *table, *data, *table2, *data2;
u32 regs[3], expected[3];
int i;
DRM_DEBUG( "%s\n", __FUNCTION__ );
table = (u32 *) dev_priv->cpu_addr_table;
/* FIXME: get a dma buffer from the freelist here rather than using the pool
*/
DRM_DEBUG( "Allocating data memory ...\n" );
cpu_addr_data = pci_pool_alloc( dev_priv->pool, SLAB_ATOMIC, &data_handle );
cpu_addr_data2 = pci_pool_alloc( dev_priv->pool, SLAB_ATOMIC, &data2_handle );
cpu_addr_table2 = pci_pool_alloc( dev_priv->pool, SLAB_ATOMIC, &table2_handle
);
if (!cpu_addr_data || !data_handle || !cpu_addr_data2 || !data2_handle ||
!cpu_addr_table2 || !table2_handle) {
DRM_INFO( "data-memory allocation failed!\n" );
return -ENOMEM;
} else {
data = (u32 *) cpu_addr_data;
data_addr = (u32) data_handle;
data2 = (u32 *) cpu_addr_data2;
data2_addr = (u32) data2_handle;
table2 = (u32 *) cpu_addr_table2;
table2_addr = (u32) table2_handle;
}
DRM_INFO( "data1: 0x%08x data2: 0x%08x\n", data_addr, data2_addr );
DRM_INFO( "table2: 0x%08x\n", table2_addr );
MACH64_WRITE( MACH64_SRC_CNTL, 0x00000000 );
MACH64_WRITE( MACH64_VERTEX_1_S, 0x00000000 );
MACH64_WRITE( MACH64_VERTEX_1_T, 0x00000000 );
MACH64_WRITE( MACH64_VERTEX_1_W, 0x00000000 );
for (i=0; i < 3; i++) {
DRM_DEBUG( "(Before DMA Transfer) reg %d = 0x%08x\n", i,
MACH64_READ( (MACH64_VERTEX_1_S + i*4) ) );
}
/* 1_90 = VERTEX_1_S, setup 3 sequential reg writes */
/* use only s,t,w vertex registers so we don't have to mask any results */
data[0] = cpu_to_le32(0x00020190);
data[1] = 0x11111111;
data[2] = 0x22222222;
data[3] = 0x33333333;
data[4] = cpu_to_le32(DMAREG(MACH64_BM_GUI_TABLE_CMD));
data[5] = cpu_to_le32(table2_addr | MACH64_CIRCULAR_BUF_SIZE_16KB);
data[6] = cpu_to_le32(0x00020190);
data[7] = 0x44444444;
data[8] = 0x55555555;
data[9] = 0x66666666;
data[10] = cpu_to_le32(0x00020190);
data[11] = expected[0] = 0x77777777;
data[12] = expected[1] = 0x88888888;
data[13] = expected[2] = 0x99999999;
data2[0] = cpu_to_le32(0x00020190);
data2[1] = 0xaaaaaaaa;
data2[2] = 0xbbbbbbbb;
data2[3] = 0xcccccccc;
DRM_DEBUG( "Preparing table ...\n" );
table[0] = cpu_to_le32(MACH64_BM_ADDR + APERTURE_OFFSET);
table[1] = cpu_to_le32(data_addr);
table[2] = cpu_to_le32(6 * sizeof( u32 ) | 0x40000000);
table[3] = 0;
table[4] = cpu_to_le32(MACH64_BM_ADDR + APERTURE_OFFSET);
table[5] = cpu_to_le32(data_addr + 6 * sizeof(u32));
table[6] = cpu_to_le32(4 * sizeof( u32 ) | 0x40000000);
table[7] = 0;
table[8] = cpu_to_le32(MACH64_BM_ADDR + APERTURE_OFFSET);
table[9] = cpu_to_le32(data_addr + 10 * sizeof(u32));
table[10] = cpu_to_le32(4 * sizeof( u32 ) | 0x80000000 | 0x40000000);
table[11] = 0;
table2[0] = cpu_to_le32(MACH64_BM_ADDR + APERTURE_OFFSET);
table2[1] = cpu_to_le32(data2_addr);
table2[2] = cpu_to_le32(4 * sizeof( u32 ) | 0x80000000 | 0x40000000);
table2[3] = 0;
DRM_DEBUG( "table[0] = 0x%08x\n", table[0] );
DRM_DEBUG( "table[1] = 0x%08x\n", table[1] );
DRM_DEBUG( "table[2] = 0x%08x\n", table[2] );
DRM_DEBUG( "table[3] = 0x%08x\n", table[3] );
for ( i = 0 ; i < 6 ; i++) {
DRM_DEBUG( " data[%d] = 0x%08x\n", i, data[i] );
}
mach64_flush_write_combine();
DRM_DEBUG( "waiting for idle...\n" );
if ( ( i = mach64_do_wait_for_idle( dev_priv ) ) ) {
DRM_INFO( "mach64_do_wait_for_idle failed (result=%d)\n", i);
DRM_INFO( "resetting engine ...\n");
mach64_do_engine_reset( dev_priv );
DRM_INFO( "freeing data buffer memory.\n" );
pci_pool_free( dev_priv->pool, cpu_addr_data, data_handle );
pci_pool_free( dev_priv->pool, cpu_addr_data2, data2_handle );
pci_pool_free( dev_priv->pool, cpu_addr_table2, table2_handle );
DRM_INFO( "returning ...\n" );
return i;
}
DRM_DEBUG( "waiting for idle...done\n" );
DRM_DEBUG( "BUS_CNTL = 0x%08x\n", MACH64_READ( MACH64_BUS_CNTL ) );
DRM_DEBUG( "SRC_CNTL = 0x%08x\n", MACH64_READ( MACH64_SRC_CNTL ) );
DRM_DEBUG( "\n" );
DRM_DEBUG( "data bus addr = 0x%08x\n", data_addr );
DRM_DEBUG( "table bus addr = 0x%08x\n", dev_priv->table_addr );
DRM_INFO( "starting DMA transfer...\n" );
MACH64_WRITE( MACH64_BM_GUI_TABLE_CMD,
dev_priv->table_addr |
MACH64_CIRCULAR_BUF_SIZE_16KB );
MACH64_WRITE( MACH64_SRC_CNTL,
MACH64_SRC_BM_ENABLE | MACH64_SRC_BM_SYNC |
MACH64_SRC_BM_OP_SYSTEM_TO_REG );
/* Kick off the transfer */
DRM_DEBUG( "starting DMA transfer... done.\n" );
MACH64_WRITE( MACH64_DST_HEIGHT_WIDTH, 0 );
DRM_INFO( "waiting for idle...\n" );
if ( ( i = mach64_do_wait_for_idle( dev_priv ) ) ) {
/* engine locked up, dump register state and reset */
DRM_INFO( "mach64_do_wait_for_idle failed (result=%d)\n", i);
mach64_dump_engine_info( dev_priv );
DRM_INFO( "resetting engine ...\n");
mach64_do_engine_reset( dev_priv );
DRM_INFO( "freeing data buffer memory.\n" );
pci_pool_free( dev_priv->pool, cpu_addr_data, data_handle );
pci_pool_free( dev_priv->pool, cpu_addr_data2, data2_handle );
pci_pool_free( dev_priv->pool, cpu_addr_table2, table2_handle );
DRM_INFO( "returning ...\n" );
return i;
}
DRM_INFO( "waiting for idle...done\n" );
mach64_dump_engine_info( dev_priv );
/* Check register values to see if the GUI master operation succeeded */
for ( i = 0; i < 3; i++ ) {
regs[i] = MACH64_READ( (MACH64_VERTEX_1_S + i*4) );
DRM_INFO( "(After DMA Transfer) reg %d = 0x%08x\n", i, regs[i] );
DRM_DEBUG( "(After DMA Transfer) reg %d = 0x%08x\n", i, regs[i] );
if (regs[i] != expected[i]) {
pci_pool_free( dev_priv->pool, cpu_addr_data, data_handle );
pci_pool_free( dev_priv->pool, cpu_addr_data2, data2_handle );
pci_pool_free( dev_priv->pool, cpu_addr_table2, table2_handle
);
return -1; /* GUI master operation failed */
}
}
DRM_DEBUG( "freeing data buffer memory.\n" );
pci_pool_free( dev_priv->pool, cpu_addr_data, data_handle );
pci_pool_free( dev_priv->pool, cpu_addr_data2, data2_handle );
pci_pool_free( dev_priv->pool, cpu_addr_table2, table2_handle );
DRM_DEBUG( "returning ...\n" );
return 0;
}
